[PATCH] D65966: AMDGPU/SILoadStoreOptimizer: Improve merging of out of order offsets

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle accepted this revision.
nhaehnle added a comment.
This revision is now accepted and ready to land.

Thanks. This basically looks good to me, some minor nitpicks.



================
Comment at: llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:772
 
+  // Handle SMEM and VMEM instructions.
   // If the offset in elements doesn't fit in 8-bits, we might be able to use
----------------
Should be "Handle DS instructions."


================
Comment at: llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:838-839
+/// This function assumes that CI comes before Paired in a basic block.
 bool SILoadStoreOptimizer::findMatchingInst(CombineInfo &CI,
                                             CombineInfo &Paired) {
+
----------------
This method should probably be renamed now since its meaning has shifted by quite a lot. Not sure what name is best, maybe `checkAndPrepareMerge`? The "prepare" part refers to collecting the InstsToMove.

Come to think of it, maybe the InstsToMove could be moved out of the CombineInfo struct? Then the CombineInfo arguments here could almost be passed as const, except for the fixup by the final offsetsCanBeCombined.


================
Comment at: llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:1991-1992
+    std::list<CombineInfo> &MergeList = *I;
+    if (MergeList.empty()) {
+      I = MergeableInsts.erase(I);
       continue;
----------------
Maybe move this check into `optimizeInstsWithSameBaseAddr`? That function is already responsible for handling the case where the size is 1...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65966