[PATCH] D116574: Materializing constants with 'rori'

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:149
+  // for case: 0xffffff77ffffffff
+  if (LeadingOnes < 32 && ((64 - LeadingOnes - TrailingOnes) < 12)) {
+    NegImm12 = ((uint64_t)Val >> TrailingOnes) | (Val << (64 - TrailingOnes));
----------------
jrtc27 wrote:
> craig.topper wrote:
> > Can we do this for LeadingOnes == 32 too? For example 0xffffffff77ffffff. That can't use LUI+ADDIW.
> Doesn't this work whenever LeadingOnes + TrailingOnes > (64 - 12) and TrailingOnes > 0 (which is currently implied by your conditions, somewhat non-obviously)? I don't see why you need LeadingOnes < 32, other than to avoid the case when the input is all 1's or all 0's and you end up trying to shift by 64, i.e. also TrailingOnes < 64.
As I said you need to take care when the input is all 1s, i.e. check say TrailingOnes < 64, otherwise you have UB due to shifting a uint64_t by 64


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:157-158
+    // for case: 0xaffffffffffffffa
+    if (UpperTrailingOnes < 32 && LowerLeadingOnes < 32 &&
+        ((64 - UpperTrailingOnes - LowerLeadingOnes) < 12)) {
+      NegImm12 = (Val << (32 - UpperTrailingOnes)) |
----------------
BaoshanPang wrote:
> jrtc27 wrote:
> > Again this seems overly strict, you just need LowerLeadingOnes + UpperTrailingOnes > (64 - 12) and 0 < UpperTrailingOnes < 32?
> Yes, I agree we should only use "LowerLeadingOnes + UpperTrailingOnes > (64 - 12)".
> 
> For 0 < UpperTrailingOnes < 32, it seems we don't need it, as:
> if UpperTrailingOnes is 0 then it is impossible to meet: LowerLeadingOnes + UpperTrailingOnes > (64 - 12)
> and if UpperTrailingOnes is 32 then the 'Val' should be handled by case 1.
I don't like relying on that, it's fragile, and not obvious because nowhere does it say that's a condition. It should at //least// be an assertion.

Except, once you correctly handle all-ones input for the first case, i.e. don't let it match as that would cause UB in the shifts, you will end up falling through to here, and you do need to explicitly check UpperTrailingOnes < 32, otherwise this will be broken too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116574