tahonermann 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; ---------------- Manna wrote: > sdesmalen wrote: > > erichkeane wrote: > > > 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? > > There's nothing that actively prevents it, but `encodeFlag` is a utility > > function that has no uses outside this file and has only 4 uses. Adding an > > assert should be sufficient. > Thank you for the explanation! I'm not sure if asserting `Mask != 0` will suffice to silence Coverity. While Coverity can specifically observe that `countr_zero` might return 0 (because `TrailingZerosCounter<T, 8>::count()` has a `return 64` statement), I don't think Coverity can determine that the function can't return 65 or higher. I think Erich's initial intuition is correct; the concern that Coverity is reporting is that the shift might overflow, so that is what should be guarded. assert(Shift < 64 && "Mask value produced an invalid shift value"); CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150140/new/ https://reviews.llvm.org/D150140 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits