[PATCH] D135137: [AggressiveInstCombine] Load merge the reverse load pattern and mixed load sizes.

[Biplob Mishra via Phabricator via llvm-commits]

bipmis marked 2 inline comments as done.
bipmis added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:724
   uint64_t LoadSize2 = LI2->getType()->getPrimitiveSizeInBits();
-  if (Load1Ptr != Load2Ptr || LoadSize1 != LoadSize2)
+  if (Load1Ptr != Load2Ptr)
     return false;
----------------
spatel wrote:
> dmgreen wrote:
> > What happens if the load sizes are not the same? That sounds like it was protecting against a number of things.
> Related question: can we make this one-line change independently of the rest of the patch (either before or after)? That would reduce risk that we uncover some difficult-to-debug corner case.
This is from the review comments from initial Implementation to enable mixed size merge. Some of the scenarios as added in the test seems to be working fine. However, in doing so we still make sure that the entire chain is reduced to a single load.
I am reverting this change for now. If needed this can go independently in a patch.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:724
   uint64_t LoadSize2 = LI2->getType()->getPrimitiveSizeInBits();
-  if (Load1Ptr != Load2Ptr || LoadSize1 != LoadSize2)
+  if (Load1Ptr != Load2Ptr)
     return false;
----------------
bipmis wrote:
> spatel wrote:
> > dmgreen wrote:
> > > What happens if the load sizes are not the same? That sounds like it was protecting against a number of things.
> > Related question: can we make this one-line change independently of the rest of the patch (either before or after)? That would reduce risk that we uncover some difficult-to-debug corner case.
> This is from the review comments from initial Implementation to enable mixed size merge. Some of the scenarios as added in the test seems to be working fine. However, in doing so we still make sure that the entire chain is reduced to a single load.
> I am reverting this change for now. If needed this can go independently in a patch.
That is a good suggestion. I think we can do this last bit towards the end if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135137