[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:7878-7880
 calculateByteProvider(SDValue Op, unsigned Index, unsigned Depth,
-                      bool Root = false) {
+                      Optional<uint64_t> VectorIndex,
+                      unsigned StartingIndex = 0, bool Root = false) {
----------------
I added more tests with:
ef7d61d67cb9
...so please update the auto-generated checks there.

It might help to add some test or code comments to explain the transforms in those examples. Just looking at this code, I can't tell what the relationship is between the 3 index parameters.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7892
+  // Fail to combine if we have encountered anything but a LOAD after handling
+  // and ExtractVectorElement.
+  if (Op.getOpcode() != ISD::LOAD && VectorIndex.has_value())
----------------
and -> an?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8224
     assert(P.isMemory() && "Must be a memory byte provider");
-    unsigned LoadBitWidth = P.Load->getMemoryVT().getSizeInBits();
     assert(LoadBitWidth % 8 == 0 &&
----------------
Can this just be updated to use getScalarSizeInBits()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133584