[MLIR][Quant] Added default to QuantizationFlags#844
Conversation
tr00
requested review from
ehsan-toosi,
jorickert,
ljfitz,
roberteg16 and
ttjost
as code owners
| enum FlagValue { | ||
| None = 0, | ||
| // Indicates that the storage type should be interpreted as a signed | ||
| // integer. The default is to interpret it as an unsigned value. | ||
| Signed = 1, | ||
| }; |
There was a problem hiding this comment.
IMO the even better approach would be to get rid of this flag completely, its unclear to me why it exists, it seems like it just duplicates information from the storage type
There was a problem hiding this comment.
But what about signless integer types? Then the sign is not encoded in the type.
There was a problem hiding this comment.
I would prefer to just not use signless integer type as storage type if sign of it is relevant
There was a problem hiding this comment.
I'm of the same opinion, as it's creating confusion when the storage type is itself signed, but this flag is set to 0. Should we consider storageType or override with this flag?
There was a problem hiding this comment.
Maybe as a first measure, we could add a check to verifyInvariants() in QuantTypes.cpp that ensures storageType and flag agree.
3 participants
Added QuantizationFlags::None to be more robust than unsigned integer value 0.