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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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.
----------------
Is the `*` in the sequence trying to convey something?


================
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)
----------------
Do we need to worry about ValDiff being INT64_MIN which makes std::abs undefined?


================
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);
----------------
Why is SeqStepNum cast from int64_t to uint64_t? I don't understand why it is the original code either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106533