[PATCH] D64911: [AMDGPU] Extend the SI Load/Store optimizer

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I still think we should be handling these on the IR level



================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:459-460
+      const AMDGPU::MIMGInfo *Info = AMDGPU::getMIMGInfo(Opc);
+      if (Info)
+        return Info->BaseOpcode;
     }
----------------
Can this actually, legitimately fail? I would expect this to be an assert


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:498
+    } else if (TII->isMIMG(Opc)) {
+      /* Ignore instructions encoded without vaddr */
+      if (AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vaddr) == -1)
----------------
C++ style comment


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:1114-1115
+  if (CI.InstClass == MIMG) {
+    assert("No overlaps" &&
+           (countPopulation(CI.DMask0 | CI.DMask1) == CI.Width0 + CI.Width1));
+    ReverseOrder = CI.DMask0 > CI.DMask1;
----------------
Weird to put the assert string first 


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:1120-1125
+  unsigned Idxs [4][4] = {
+      {AMDGPU::sub0, AMDGPU::sub0_sub1, AMDGPU::sub0_sub1_sub2, AMDGPU::sub0_sub1_sub2_sub3},
+      {AMDGPU::sub1, AMDGPU::sub1_sub2, AMDGPU::sub1_sub2_sub3, 0},
+      {AMDGPU::sub2, AMDGPU::sub2_sub3, 0, 0},
+      {AMDGPU::sub3, 0, 0, 0},
+  };
----------------
static const


================
Comment at: test/CodeGen/AMDGPU/merge-image-load.mir:4
+# GFX9-LABEL: name: image_load_l_merged_v1v3
+# GFX9: %{{[0-9]+}}:vreg_128 = IMAGE_LOAD_V4_V4 %5, %3, 15, 0, 0, 0, 0, 0, 0, -1, 0, implicit $exec
+# GFX9: %{{[0-9]+}}:vgpr_32 = COPY %8.sub0
----------------
Can you include the merged MMO in the check lines


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64911