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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Please pre-commit the new tests with the baseline CHECK lines. 
Also, it would be good to add a test that matches the reported failure case (the `@eggs` test from the earlier comment).
After that, I think it's fine to re-commit.



================
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:
> 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.
Thanks for checking. It doesn't directly impact the initial/immediate patch here, but it might provide a template for the planned enhancements.


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

https://reviews.llvm.org/D127392