[PATCH] D150140: [NFC][CLANG] Fix Static Code Analysis Concerns

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150140/new/

https://reviews.llvm.org/D150140