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

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

nhaehnle added a comment.

I think the code would benefit from the refactoring I've mentioned on the other patch, where the lists only hold a structure with information on a single instruction. Maybe call it CandidateInfo (information of one instruction, persistent in lists) vs. CombineInfo (information on a pair, only temporary).



================
Comment at: llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:482-487
+  // We can't pair these if we can't determine the instruction
+  // class.
+  if (InstClass == UNKNOWN || getInstClass(MI->getOpcode(), TII) != InstClass) {
+    InstClass = UNKNOWN;
+    return;
+  }
----------------
Can that really happen? Only instructions with the same InstClass should be added to the same list.


================
Comment at: llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:1595
+                                     MachineBasicBlock::const_iterator Second) const {
+  // FIXME: Is there a better way to do this.
+  const MachineBasicBlock *MBB = First->getParent();
----------------
arsenm wrote:
> Don't you know which is first from which was encountered first?
Wasn't there some talk about ordered basic blocks? They exist for IR apparently, but not for MIR unless we're tracking live ranges, which we don't do here, so...

This pass could perhaps number the CombineInfo instructions in order as they're collected at the start? It'd have to be kept uptodate as instructions are merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65966