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

[Erich Keane via Phabricator via cfe-commits]

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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150140