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

Fri Dec 6 18:59:38 PST 2019

tstellar marked 4 inline comments as done.
tstellar added a comment.

In D65966#1701129 <https://reviews.llvm.org/D65966#1701129>, @nhaehnle wrote:

> 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).

See D71045 <https://reviews.llvm.org/D71045> .

================
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();
----------------
nhaehnle wrote:
> 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.
I added an Order field to keep track of the ordering.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65966