This is an archive of the discontinued LLVM Phabricator instance.

[CGP] Make optimizeMemoryInst able to combine more kinds of ExtAddrMode fields
ClosedPublic

Authored by john.brawn on Sep 21 2017, 7:22 AM.

Details

Summary

This patch extends the recent work in optimizeMemoryInst to make it able to combine more ExtAddrMode fields than just the BaseReg.

This fixes some benchmark regressions introduced by r309397, where GVN PRE is hoisting a getelementptr such that it can no longer be combined into the addressing mode of the load or store that uses it.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn created this revision.Sep 21 2017, 7:22 AM
davidxl added inline comments.Sep 21 2017, 9:55 AM
test/Transforms/CodeGenPrepare/ARM/sink-addrmode.ll
160 ↗(On Diff #116182)

The old select may have profile meta data, so you need to make sure the new select inherits it.

skatkov edited edge metadata.Sep 22 2017, 2:02 AM

Do I understand correctly that you consider only very simple cases?
Specifically if Addr is represented as Select or Phi and no one from its input might be other Select/Phi.

Also it seems you create Phi unconditionally even if the Phi you need already exists.

Please take a look into the patch I come up to cover more cases https://reviews.llvm.org/D36073 but for Phi nodes only and for base registers only.

Does it makes sense for you to review it and extend with select support and other than base parts?

lib/CodeGen/CodeGenPrepare.cpp
4388 ↗(On Diff #116182)

What if this Phi already exists?

4581 ↗(On Diff #116182)

For select you set PhiSeen to true so you will skip this check but for select all intructions might be in the same basic block. Is it what you expect?
If yes, you need to update a comment.

4617 ↗(On Diff #116182)

bail out earlier?

reames edited edge metadata.Sep 22 2017, 2:19 PM

Does it makes sense for you to review it and extend with select support and other than base parts?

I will insist on collaboration here. It doesn't have to be the case that we land either patch as currently structured; we could instead extract common functionality they can both build on. We don't want to waste the time either of you have put into this, and as currently structured the two patches are conflicting. That needs to change.

reames added inline comments.Sep 22 2017, 2:44 PM
lib/CodeGen/CodeGenPrepare.cpp
4388 ↗(On Diff #116182)

I think this case where a single PHI or Select needs inserted (or reused) is a enough to require the appropriate structure in the caller. I'd suggest that we factor out a common patch which does this which can implement both your patch and Serguei's analysis behind the same API. Once we have that, the code structure is in place for either patch and we can focus on reviewing the two analyses.

4498 ↗(On Diff #116182)

I think the select handling can be separate from the rest of the patch and reviewed separately. This should kick in even if the underlying addresses share the same addr mode.

Please do this to simply the review and increase the common code which can be shared between the two related patches in this area.

4558 ↗(On Diff #116182)

Having an unbounded number of AddrModes here is likely problematic. How much power do you loose if you cap this to say 2 or 3?

reames requested changes to this revision.Sep 22 2017, 2:50 PM

Marking as changes needs to reflect split requested. Note that I really haven't even looked at the guts of the transform and don't plan to until the common pieces have been extracted.

This revision now requires changes to proceed.Sep 22 2017, 2:50 PM

OK, so looking at the comments here and looking at D36073 what looks like it would be the easiest way to progress things forwards is:

  • I split out the select handling into a separate patch.
  • I split out a patch which gathers the AddrModes and figures out how they differ, but then bails out when we get to sinking if we have more than one.
  • @skatkov modifies D36073 to build on top of this - from a brief look at FindCommonBase and the functions they call it looks like they could be modified to handle taking the AddrModes vector, so then we can handle the DifferentBase case similar to as in this patch but with AddrMode.BaseReg = FindCommonBase() instead of AddrMode.BaseReg = CreateSelectOrPhi().
  • I then write a patch on top of that which modifies FindCommonBase to handle the fields other than the base register.

Does this seem reasonable?

john.brawn added inline comments.Sep 25 2017, 10:12 AM
lib/CodeGen/CodeGenPrepare.cpp
4498 ↗(On Diff #116182)

I've split this out into D38242.

OK, so looking at the comments here and looking at D36073 what looks like it would be the easiest way to progress things forwards is:

  • I split out the select handling into a separate patch.
  • I split out a patch which gathers the AddrModes and figures out how they differ, but then bails out when we get to sinking if we have more than one.
  • @skatkov modifies D36073 to build on top of this - from a brief look at FindCommonBase and the functions they call it looks like they could be modified to handle taking the AddrModes vector, so then we can handle the DifferentBase case similar to as in this patch but with AddrMode.BaseReg = FindCommonBase() instead of AddrMode.BaseReg = CreateSelectOrPhi().
  • I then write a patch on top of that which modifies FindCommonBase to handle the fields other than the base register.

Does this seem reasonable?

Yes, it seems reasonable to me.

OK, so looking at the comments here and looking at D36073 what looks like it would be the easiest way to progress things forwards is:

  • I split out the select handling into a separate patch.
  • I split out a patch which gathers the AddrModes and figures out how they differ, but then bails out when we get to sinking if we have more than one.
  • @skatkov modifies D36073 to build on top of this - from a brief look at FindCommonBase and the functions they call it looks like they could be modified to handle taking the AddrModes vector, so then we can handle the DifferentBase case similar to as in this patch but with AddrMode.BaseReg = FindCommonBase() instead of AddrMode.BaseReg = CreateSelectOrPhi().
  • I then write a patch on top of that which modifies FindCommonBase to handle the fields other than the base register.

Does this seem reasonable?

Yes, it seems reasonable to me.

To be clear, I'm waiting you for implementation of diff between AddrModes and then I update D36073 on top of your change.

john.brawn updated this revision to Diff 117365.Oct 2 2017, 9:21 AM
john.brawn edited edge metadata.

I've updated the patch to something that builds on top of D38133, to give more context to that. I'm not intending to commit this patch, as instead I'll be rewriting it to apply on top of D36073 (once that's been updated to apply on top of D38133).

john.brawn retitled this revision from [CGP] Make optimizeMemoryInst introduce a select/phi if it improves things to [CGP] Make optimizeMemoryInst able to combine more kinds of ExtAddrMode fields.
john.brawn edited the summary of this revision. (Show Details)

Reworked this so that it applies on top of D36073.

Hi John, in general looks good to me. I wonder whether we can add some options to enable/disable different fields and enable them step-by-step?
The patch has a significant impact and in this case people can use these options to quickly detect where the problem is. Does it make sense?

Please postpone the landing of this patch until I finish enabling the optimization itself.

Rebase, and allow combining of the different parts of the addressing mode to be individually disabled.

Hi John, LGTM.

Unfortunately I still have problems enabling the optimization by default. It causes build bot hangs-up, I'm evaluating the failures.
If you can also look into it, would be great.

Anyway, I think you can land this patch with all options off be default. Then I will switch them on one by one resolving different issues.

Unfortunately I still have problems enabling the optimization by default. It causes build bot hangs-up, I'm evaluating the failures.
If you can also look into it, would be great.

I've done some testing and there appears to be a problem with how isTrivial handles null pointers, I'll post a patch for that shortly.

Anyway, I think you can land this patch with all options off be default. Then I will switch them on one by one resolving different issues.

DisableComplexAddrModes already disables everything, so I don't think there's a need to disable the various AddrSinkCombine options for now. We can disable individual ones when DisableComplexAddrModes is turned on, if we need to.

This revision was automatically updated to reflect the committed changes.