[PATCH] D107790: [RISCV] Add a pass to recognize VLS strided loads/store from gather/scatter.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVGatherScatterLowering.cpp:304
+
+  int VecOperand = -1;
+  unsigned TypeScale = 0;
----------------
I think this might be clearer as `Optional<unsigned>`.


================
Comment at: llvm/lib/Target/RISCV/RISCVGatherScatterLowering.cpp:344
+
+  unsigned IncrementingBlock = BasePhi->getOperand(0) == Inc ? 0 : 1;
+
----------------
I think this //might// be guaranteed by loop simplify form, but here and/or in `matchStridedRecurrence` I think it might be best to double-check (or bail) if the PHI does/doesn't have exactly two operands or even that the incrementing block is what we expect.


================
Comment at: llvm/lib/Target/RISCV/RISCVGatherScatterLowering.cpp:373
+
+  auto BaseAndStride = std::make_pair(BasePtr, Stride);
+
----------------
no need for new variable?


================
Comment at: llvm/lib/Target/RISCV/RISCVGatherScatterLowering.cpp:444
+    for (Instruction &I : BB) {
+      // Do an initial optimization pass to push out as much address arithmetic
+      // as possible to get a more canonical IR.
----------------
Is this comment right, or in the wrong place?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:923
+    Info.opc = ISD::INTRINSIC_W_CHAIN;
+    Info.ptrVal = nullptr;
+    Info.memVT = MVT::getVT(I.getType()->getScalarType());
----------------
No `ptrVal`? We have a single scalar pointer operand so can't we use that? Or is it something to do with semantics of setting `ptrVal` that I'm not aware of? Just I'd have thought that `size` being "unknown/max" we'd prevent any bad AA.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:926
+    Info.align = Align(I.getType()->getScalarSizeInBits() / 8);
+    Info.size = ~UINT64_C(0);
+    Info.flags |= MachineMemOperand::MOLoad;
----------------
I think we can use `MemoryLocation::UnknownSize` here now. I seem to remember a case a year or so back where `IntrinsicInfo`'s `size` was `unsigned`. so couldn't represent the right value. That was fun.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107790