[PATCH] D106533: [RISCV] Support simple fractional steps in matching VID sequences

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1444
+      // A zero-value value difference means that we're somewhere in the middle
+      // of a fractional step, e.g. <0,0,0*,0,1,1,1,1>. Wait until we notice a
+      // step change before evaluating the sequence.
----------------
craig.topper wrote:
> Is the `*` in the sequence trying to convey something?
It's supposed to convey that we might currently be at that element to demonstrate the code comment, but I agree it's not very clear.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1448
+        // The difference must cleanly divide the element span.
+        if (std::abs(ValDiff) >= IdxDiff) {
+          if (ValDiff % IdxDiff != 0)
----------------
craig.topper wrote:
> Do we need to worry about ValDiff being INT64_MIN which makes std::abs undefined?
Oh good catch, thanks. I'm surprised my sanitizer build didn't hit that. Sounds like a gap in my testing.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1470
+      uint64_t ExpectedVal =
+          (int64_t)(Idx * (uint64_t)*SeqStepNum) / *SeqStepDenom;
+      int64_t Addend = SignExtend64(Val - ExpectedVal, EltSizeInBits);
----------------
craig.topper wrote:
> Why is SeqStepNum cast from int64_t to uint64_t? I don't understand why it is the original code either.
It's very possible I went overboard in trying to prevent signed integer overflow e.g. `INT64_MIN`. I could swear the sanitizers were complaining at me though.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/interleave-crash.ll:135
 ; RV64-1024-NEXT:    vmv.v.i v8, 0
+; RV64-1024-NEXT:    csrr a2, vlenb
+; RV64-1024-NEXT:    slli a2, a2, 4
----------------
This test sadly decides to spill even more than it did previously, meaning it's technically a regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106533