[PATCH] D147713: [RISCV] Combine concat_vectors of loads into strided loads

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11367
+  case ISD::CONCAT_VECTORS: {
+    SDLoc DL(N);
+    EVT VT = N->getValueType(0);
----------------
Can we move this to a function? This is a lot of code to dump into the switch. We've been pretty sloppy about this.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11369
+    EVT VT = N->getValueType(0);
+    // Only perform this combine on legal MVT types.
+    if (!isTypeLegal(VT))
----------------
Drop MVT from this comment. It's redundant with "types".


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11382
+    auto *BaseLd = dyn_cast<LoadSDNode>(N->getOperand(0));
+    if (!BaseLd || !BaseLd->isSimple() || !SDValue(BaseLd, 0).hasOneUse())
+      break;
----------------
You probably want to check this isn't an extending load either. isNormalLoad should do it I think.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11396
+            Ld->getChain() != BaseLd->getChain() ||
+            Ld->getAlign() != BaseLd->getAlign() ||
+            Ld->getValueType(0) != BaseLdVT)
----------------
We might want to find common alignment instead?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11424
+      if (ConstStride->getZExtValue() == BaseLdVT.getFixedSizeInBits() / 8) {
+        SDValue WideLoad =
+            DAG.getLoad(VT, DL, BaseLd->getChain(), BasePtr,
----------------
This also needs allowsMemoryAccessForAlignment right?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11441
+    else if (BaseLdVT.isFloatingPoint())
+      WideScalarVT = MVT::getFloatingPointVT(WideScalarBitWidth);
+    else
----------------
Wondering if we should always do integer in case f64 vector isn't supported, but i64 is?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11485
+      DAG.makeEquivalentMemoryOrdering(cast<LoadSDNode>(Ld), StridedLoad);
+    return DAG.getBitcast(
+        VT, convertFromScalableVector(WideVecVT, StridedLoad, DAG, Subtarget));
----------------
I'm wondering if the bitcast should come before the convertFromScalableVector. That keeps convertFromScalableVector/convertToScalableVector pairs together without having bitcasts between them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147713