erichkeane added inline comments.
================
Comment at: clang/utils/TableGen/SveEmitter.cpp:302
unsigned Shift = llvm::countr_zero(Mask);
+ assert(Shift >= 64 && "Shift is out of encodable range");
return (V << Shift) & Mask;
----------------
sdesmalen wrote:
> erichkeane wrote:
> > Shouldn't this be: `assert(Shift < 64 &&"...")`?
> >
> > `expr.shift` (https://eel.is/c++draft/expr.shift) says:
> > ```
> > The operands shall be of integral or unscoped enumeration type and integral
> > promotions are performed.
> > The type of the result is that of the promoted left operand.
> > The behavior is undefined if the right operand is negative, or greater than
> > or equal to the width of the promoted left operand.```
> >
> > uint64 stays as an `unsigned long`, so it is still 64 bits, so the only
> > invalid value for `Shift` is 64 (though >64 is 'nonsense', but only
> > impossible because of `llvm::countr_zero`).
> >
> > One thing to consider: I wonder if we should instead be changing the
> > 'shift' to be:
> >
> > `(V << (Shift % 64)) && Mask` ? It looks like `arm_sve.td` has the
> > `NoFlags` value as zero, which I think will end up going through here
> > possibly (or at least, inserted into `FlagTypes`.
> >
> > So I suspect an assert might not be sufficient, since a 64 bit shift is
> > possible in that case (since a zero 'Mask' is the only case where
> > `countr_zero` will end up being 64).
> >
> >
> > So I suspect an assert might not be sufficient, since a 64 bit shift is
> > possible in that case (since a zero 'Mask' is the only case where
> > countr_zero will end up being 64).
> It should be fine to assert that `Mask != 0`, since that would be an invalid
> mask.
Thanks for the comment @sdesmalen! Is there something that prevents the
`NoFlags` from being passed as the `MaskName` here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150140/new/
https://reviews.llvm.org/D150140
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
