[PATCH] D116574: Materializing constants with 'rori'

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

Shouldn't this test just go in imm.ll?



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:148
+  int TrailingOnes = countTrailingOnes((uint64_t)Val);
+  // for case: 0xffffff77ffffffff
+  if (LeadingOnes < 32 && ((64 - LeadingOnes - TrailingOnes) < 12)) {
----------------
This would be clearer written in binary (with ... for the long sequence of 1's) and x's for the interesting bits


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


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:150
+  if (LeadingOnes < 32 && ((64 - LeadingOnes - TrailingOnes) < 12)) {
+    NegImm12 = ((uint64_t)Val >> TrailingOnes) | (Val << (64 - TrailingOnes));
+    Rotate = 64 - TrailingOnes;
----------------
Need to cast Val to uint64_t when left-shifting too otherwise it's UB; left-shifting on signed types is only defined if the LHS is non-negative and LHS * 2^RHS computed as a natural number is representable in the integral type used, i.e. you're only shifting out 0s at the top and there's at least one left over.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:152
+    Rotate = 64 - TrailingOnes;
+    Ret = true;
+  } else {
----------------
Just return true


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:154
+  } else {
+    int UpperTrailingOnes = countTrailingOnes((uint64_t)(Val >> 32));
+    int LowerLeadingOnes = countLeadingOnes((uint64_t)(Val << 32));
----------------
With early return you can avoid the nesting


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:155
+    int UpperTrailingOnes = countTrailingOnes((uint64_t)(Val >> 32));
+    int LowerLeadingOnes = countLeadingOnes((uint64_t)(Val << 32));
+    // for case: 0xaffffffffffffffa
----------------
This is UB in a lot of cases, left shifts on signed values are perilous


================
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)) |
----------------
Again this seems overly strict, you just need LowerLeadingOnes + UpperTrailingOnes > (64 - 12) and 0 < UpperTrailingOnes < 32?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:162
+      Rotate = 32 - UpperTrailingOnes;
+      Ret = true;
+    }
----------------
Ditto


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp:165
+  }
+  return Ret;
+}
----------------
Just return false


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116574