[PATCH] D127392: [AggressiveInstCombine] Combine consecutive loads which are being merged to form a wider load.

[Biplob Mishra via Phabricator via llvm-commits]

bipmis added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:694-697
+  // Swap loads if LI1 comes later as we handle only forward loads.
+  // This is done as InstCombine folds lowest node forward loads to reverse.
+  // The implementation will be subsequently extended to handle all reverse
+  // loads.
----------------
bipmis wrote:
> spatel wrote:
> > I didn't notice this limitation before. "Forward" and "reverse" are referring to the order that the `or` instructions use the loaded values?
> > 
> > I agree that we don't want to complicate the initial implementation any more than necessary, but it might help to see how the backend handles that in DAGCombiner::MatchLoadCombine() (see D133584 for a proposed enhancement to that code).
> Right it is the order
> Forward - or(or(or(or(0,1),2),3)
> Reverse - or(or(or(or(3,2),1),0)
> Considering 0,1...as zext and shl of loads with index 0,1 etc.
> 
> For simplicity we wanted to implement the forward first and if this looks good we can do the reverse+mixed size loads. There should be minimal changes on top of this.  So that is in plan next.
> I didn't notice this limitation before. "Forward" and "reverse" are referring to the order that the `or` instructions use the loaded values?
> 
> I agree that we don't want to complicate the initial implementation any more than necessary, but it might help to see how the backend handles that in DAGCombiner::MatchLoadCombine() (see D133584 for a proposed enhancement to that code).

@spatel Verified DAGCombiner::MatchLoadCombine() handles the reverse load pattern fine and a single load is generated.


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

https://reviews.llvm.org/D127392