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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 2 inline comments as done.
frasercrmck added inline comments.


================
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:
> frasercrmck wrote:
> > 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.
> Good point. I guess I'm only used to thinking about addition/subtract overflow not multiply overflow.
Yeah to be honest I was surprised I hadn't ever really had to think about it before. I think I'm doing the right thing here but I wondering if it could be cleaner somehow.


================
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
----------------
craig.topper wrote:
> frasercrmck wrote:
> > This test sadly decides to spill even more than it did previously, meaning it's technically a regression.
> I guess that's because we have vid and vid/2 both live?
That'd be my guess, yeah. It's keeping `vid` alive in `v24` for longer when there aren't really that many registers available: 4, I think. It may even work out cheaper to "rematerialize" it later nearer the use. I don't know how in control we are of those decisions though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106533