This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] Teach Chain Analysis about BaseIndexOffset addressing.
ClosedPublic

Authored by niravd on Apr 12 2017, 11:37 AM.

Details

Summary

While we use BaseIndexOffset in FindBetterNeighborChains to appropriately realize they're almost the same address and should be improved concurrently we do not use it in isAlias using the non-index understanding FindBaseOffset instead. Adding a BaseIndexOffset check in isAlias like should allow indexed stores to be merged.

Once this is in we should be able to excise FindBaseOffset.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Apr 12 2017, 11:37 AM
niravd edited the summary of this revision. (Show Details)Apr 12 2017, 12:06 PM
niravd updated this revision to Diff 95576.Apr 18 2017, 8:42 AM
niravd edited the summary of this revision. (Show Details)

Rebasing to showoff test change with now committed testcase

Added a few more reviewers as jyknight is on vacation.

spatel edited edge metadata.Apr 20 2017, 11:28 AM

I had not looked at this code before, so I'll need help reviewing. As part of trying to understand what this function is doing, I tried to improve it with some cleanups:
rL300850
rL300854
rL300860

This patch says that if BaseIndexOffset::match() returns matching base pointers, then we don't need to go through all of the checks below it. In that case, can we remove some/all of the code in findBaseOffset()?

I had not looked at this code before, so I'll need help reviewing. As part of trying to understand what this function is doing, I tried to improve it with some cleanups:
rL300850
rL300854
rL300860

Nice!

This patch says that if BaseIndexOffset::match() returns matching base pointers, then we don't need to go through all of the checks below it. In that case, can we remove some/all of the code in findBaseOffset()?

Yes. This should fully encompass the Base check and we should be able to repoint the relevant additional checks for globals/constants and framepointers to the baseindex check. I have a follow up patch that should completely remove findBaseOffset and do the incremental checks for FramePointers etc. off of the BaseIndexOffset match which should be correct (though I need to do a more thorough check to be more sure). I kept if off here to make it clearer that this is just adding the BaseIndexOffset case analysis to our current BaseOffset check and not dropping any cases.

niravd updated this revision to Diff 96391.Apr 24 2017, 6:41 AM

Update past Sanjay's cleanups in trunk.

spatel accepted this revision.Apr 24 2017, 7:34 AM

Yes. This should fully encompass the Base check and we should be able to repoint the relevant additional checks for globals/constants and framepointers to the baseindex check. I have a follow up patch that should completely remove findBaseOffset and do the incremental checks for FramePointers etc. off of the BaseIndexOffset match which should be correct (though I need to do a more thorough check to be more sure). I kept if off here to make it clearer that this is just adding the BaseIndexOffset case analysis to our current BaseOffset check and not dropping any cases.

In that case, can you add a 'FIXME' comment describing this plan as part of this patch. Just in case the follow-ups are delayed for some reason, we don't want to leave this code in a confusing state.
Given that, LGTM. But if anyone else can also give this a look, that would be best since I'm not that familiar with all of the aliasing possibilities.

This revision is now accepted and ready to land.Apr 24 2017, 7:34 AM
This revision was automatically updated to reflect the committed changes.