[PATCH] D133584: [DAGCombiner] [AMDGPU] Allow vector loads in MatchLoadCombine

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7894
+  // Multiple uses of vector loads are expected
+  if (!Root && !Op.hasOneUse() &&
+      !(Op.getOpcode() == ISD::LOAD && Op.getValueType().isVector()))
----------------
Check that `Depth != 0` instead of adding the Root parameter?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8293
     assert(P.isMemory() && "Must be a memory byte provider");
-    unsigned LoadBitWidth = P.Load->getMemoryVT().getSizeInBits();
+    unsigned LoadBitWidth;
+    if (P.Load->getValueType(0).isVector()) {
----------------
Still need to resolve this - if this can't be:
    unsigned LoadBitWidth = P.Load->getMemoryVT().getScalarSizeInBits();
...then please add a test to demonstrate how that fails or a code comment to explain why the simpler code is not valid.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8349-8350
+
+    // Add the VectorOffset (if any) to the offset of Ptr
+    Ptr.setOffset(Ptr.getOffset() + P->VectorOffset);
+
----------------
Do we have a test where VectorOffset is non-zero? If not, please add one (add baseline tests as needed, no pre-commit review is necessary).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133584