This is an archive of the discontinued LLVM Phabricator instance.

Fix a bug when hoist spill to a BB with landingpad successor
ClosedPublic

Authored by wmi on May 3 2016, 11:34 AM.

Details

Reviewers
qcolombet
MatzeB
Summary

This is to fix the bug in https://llvm.org/bugs/show_bug.cgi?id=27612.

When spill is hoisted to a BB with landingpad successor, and if the VNI of the spill reg lives into the landingpad successor, the
spill should be inserted before the call which may throw exception. I uses SplitAnalysis::computeLastSplitPoint to get the insert point.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi updated this revision to Diff 56037.May 3 2016, 11:34 AM
wmi retitled this revision from to Fix a bug when hoist spill to a BB with landingpad successor.
wmi updated this object.
wmi added a reviewer: qcolombet.
wmi set the repository for this revision to rL LLVM.
wmi added subscribers: llvm-commits, davidxl, echristo.
echristo added inline comments.May 3 2016, 2:42 PM
lib/CodeGen/InlineSpiller.cpp
1397

Needs a comment of why you want to skip the analysis here.

test/CodeGen/X86/hoist-spill-lpad.ll
4–6

I think this can be reworded in a more general way such that it's clear if this breaks what's going on. If I understand correctly we need to use the split point as the insert location because the spill reg could be live through the next block?

wmi added a comment.May 3 2016, 10:37 PM

Address Eric's comment.

lib/CodeGen/InlineSpiller.cpp
1397

Comments added

test/CodeGen/X86/hoist-spill-lpad.ll
4–6

Yes, the spill should live through both the fall through path and the landingpad path of the same call which may throw, so we should use the split point as the insert point. comment fixed.

wmi updated this revision to Diff 56094.May 3 2016, 10:41 PM
majnemer added inline comments.
lib/CodeGen/SplitKit.h
124–127

It is a little confusing to have a function called analyze which takes a parameter named SkipAnalyze. Perhaps it would be more obvious what you are trying to achieve if the functionality that you desired from analyze was split out into another method.

wmi updated this revision to Diff 56207.May 4 2016, 2:20 PM
wmi removed rL LLVM as the repository for this revision.

Address David's comment.

echristo added inline comments.May 4 2016, 4:53 PM
lib/CodeGen/InlineSpiller.cpp
1397

Capital 'S' at the beginning of the sentence.

Perhaps: "Use the current interval for SA to compute a safe insertion point in the target basic block."

lib/CodeGen/SplitKit.h
124–127

I'm not sure I get it after your changes. Can you explain what you're doing now with setParent in a bit more detail?

wmi added inline comments.May 4 2016, 5:06 PM
lib/CodeGen/InlineSpiller.cpp
1397

Thanks. That is a better description. will fix it.

lib/CodeGen/SplitKit.h
124–127

I only need to set current liveinterval for SplitAnalysis. I creat a method SplitAnalysis::setParent for it.

124–127

I use setParent to set SplitAnalysis::CurLi, which will be used in SplitAnalysis::computeLastSplitPoint to compute the last split point, i.e., insert point of bb.

One inline comment about the patch and a couple about the testcase. If you could look through the testcase and see if there's anything else that can be removed it'd be appreciated.

-eric

lib/CodeGen/SplitKit.h
124–127

Why aren't you just doing something like:

if (SA->getParent() != OrigLi)
  analyze(OrigLi);
SA->getLastSplitPointIter(BB);

or some such? I'm probably missing something here :)

test/CodeGen/X86/hoist-spill-lpad.ll
113

Can delete.

124

Can delete.

wmi added inline comments.May 5 2016, 9:44 AM
lib/CodeGen/SplitKit.h
124–127

Because there are more than one place (now two, but maybe more in the future) where getLastSplitPointIter is called, I choose to set CurLI for SA at the beginning.

test/CodeGen/X86/hoist-spill-lpad.ll
113

unused func decls removed.

wmi updated this revision to Diff 56305.May 5 2016, 9:45 AM
wmi set the repository for this revision to rL LLVM.
echristo added inline comments.May 5 2016, 2:17 PM
lib/CodeGen/SplitKit.h
124–127

I could be missing something but analyze sets the interval and does the analysis which is then cached, but the setParent you've got doesn't do the analysis and so isn't updating aspects of SA right (the computation is happening as part of getLastSplitPointIter, but the clear and analyzeUses isn't happening)? You might not be using it, but it seems odd to leave it in an indeterminate state.

wmi added inline comments.May 5 2016, 2:58 PM
lib/CodeGen/SplitKit.h
124–127

The cache LastSplitPoint[] in SA is not changed even if CurLI is changed, so we don't need to updated it every time we call setParent (you can see that in SplitAnalysis::clear(), LastSplitPoint is not cleared there. It is also not set by analyzeUses()).

SplitAnalysis::clear() is used to clear several caches of other analysises set up in SplitAnalysis::analyzeUses(). Those analysises are unused by hoistspill for now. To make SA a little bit light weighted. I choose not to call analyze(). If other analysises in SplitAnalysis will be used in hoistspill in the future, we can easily change setParent() to analyze() to make them available.

I will add a comment before setParent() is called to make it clear.

echristo added inline comments.
lib/CodeGen/SplitKit.h
124–127

I see what you mean. Basically getLastSplitPoint could be static if it wasn't for the cache. I worry about surprising people with bugs with setting the LI and then not resetting the rest of the object.

I think this needs a bit more documentation in general. Personally I'm not a huge fan of side-stepping this aspect of the analysis here, but I'm not sure we should pull it out or do something else either. I'm probably going to have to defer to Quentin or Matthias here.

MatzeB edited edge metadata.May 5 2016, 4:34 PM

I just filed http://llvm.org/PR27659 which we should fix in the long term.

For the short term we do indeed need code like the one found in SplitKit::computeLastSplitPoint(). However we should not need to insert the whole of SplitKit just to determine a good insertion point. I think what we should do instead is:

  • Factor the code in SplitKit::computeLastSplitPoint out into an independent function which is then used by SplitKit and the InlineSpiller.
  • I don't know how many queries the InlineSpiller will make, but maybe we can live without the caching logic in SplitKit here? Otherwise the best action would be to pull that out into a custom class as well which is then used by SplitKit and the InlinerSpiller.
qcolombet edited edge metadata.May 5 2016, 5:55 PM

Hi Wei,

I’ll second Matthias that we should not need the full split kit here and refactoring should be the way to go.
I would however like to keep the caching mechanism at least in split kit. So, like pretty much Matthias said:

  • Introduce a utility function to query the last split point.
  • Introduce a class that does the caching on top of that utility function. (That could even be an analysis pass, but that is probably overkill.)

That way if we have users that do not want the caching mechanism (for hoist spill?) they have a direct access to the utility function, otherwise they use the class (for split kit).

Couple of comments on the test case:
I believe this is a test case reduced by bug point, and I believe you can make it smaller and more robust to other changes. Basically come-up with a simple CFG pattern where the spill hoisting is triggered (a diamond should be enough, right), add a lpad at the strategic point, then add a few inlineasm nop with side effects clobbering all the registers (or at least a lot o them) to create high register pressure points where need be.

I know this is a hassle but there are reasons why I am asking this:

  1. This kind of test cases happens to work because we are not removing undef branches and such. (We could imagine this won’t stand in the future.)
  2. When the test fails because of regalloc (or other) changes, this is pretty much impossible to keep the coverage they were providing and we unfortunately just remove them :(.

Cheers,
-Quentin

test/CodeGen/X86/hoist-spill-lpad.ll
8

Could we use simpler name for the function :).

wmi added a comment.May 6 2016, 12:01 PM

Matthias, Quentin and Eric, thanks for the suggestions. I separated the extraction of LastSplitPoint into another patch. I think it is still beneficial to use cache for hoistspill case, so both SplitAnalysis and HoistSpillHelper will use the LSplitPointAnalysis class.

I will start to work on the testcase now.

wmi updated this revision to Diff 56493.May 6 2016, 10:34 PM
wmi edited edge metadata.

Create a testcase hoist-spill-lpad.ll based on hoist-spill-lpad.cc
update patch based on http://reviews.llvm.org/D20027

You should avoid unnecessary heap allocation of the analysis (see below).
As for the testcase I believe what Quentin was asking was not to create a new .cc file as a test but rather to simplify and cleanup the hoist-spill-lpad.ll according to his suggestions.

lib/CodeGen/InlineSpiller.cpp
72

This comment doesn't contain any additional information over what the user would found in the description of the InsertPointAnalysis class. I'd leave it out.

72–73

This should better be a member instead of being allocated on the heap. You can add a reset() method to InsertPointAnalysis to enable this.

wmi added a comment.May 9 2016, 1:22 PM

As for the testcase I believe what Quentin was asking was not to create a new .cc file as a test but rather to simplify and cleanup the hoist-spill-lpad.ll according to his suggestions.

hoist-spill-lpad.ll is a new testcase generated from the .cc file. I uploaded the .cc file too for easier review. I think the
new hoist-spill-lpad.ll testcase will be relatively stable for other compiler changes.

Thanks,
Wei.

lib/CodeGen/InlineSpiller.cpp
72

will remove it.

72–73

ok, I will make InsertPointAnalysis a member of HoistSpillHelper. reset() is not needed because the lifetimes of InsertPointAnalysis, the LastSplitPoint[] cache and HoistSpillHelper will be the same. When we only reset InsertPointAnalysis::CurLI, the cache is still valid.

wmi updated this revision to Diff 56635.May 9 2016, 2:37 PM

Make InsertPointAnalysis a member in class HoistSpillHelper.

So does the LastInsertPointAnalysis cache depend on the current LiveInterval?

  • If so then we should not make it a member of the analysis class and just pass the liveinterval as a parameter in each query.
  • If however the cache does depend on the current interval then we miss a cache clear here when we change the current interval.
wmi added a subscriber: wmi.May 11 2016, 3:07 PM

So does the LastInsertPointAnalysis cache depend on the current LiveInterval?

  • If so then we should not make it a member of the analysis class and just pass the liveinterval as a parameter in each query.
  • If however the cache does depend on the current interval then we miss a cache clear here when we change the current interval.

No, LastInsertPoint cache doesn't depend on current LiveInterval. The
cache only records the two possible insert points of a BB - Before
BB's FirstTerminator, and before the last call if BB has landingpad
successor, which are unrelated with LiveInterval. The cache is used to
accelerate the determination of the final insert point to insert
spill/split for current LiveInterval.

Thanks,
Wei.

MatzeB accepted this revision.May 11 2016, 3:09 PM
MatzeB edited edge metadata.
In D19884#427795, @wmi wrote:

So does the LastInsertPointAnalysis cache depend on the current LiveInterval?

  • If so then we should not make it a member of the analysis class and just pass the liveinterval as a parameter in each query.
  • If however the cache does depend on the current interval then we miss a cache clear here when we change the current interval.

No, LastInsertPoint cache doesn't depend on current LiveInterval. The
cache only records the two possible insert points of a BB - Before
BB's FirstTerminator, and before the last call if BB has landingpad
successor, which are unrelated with LiveInterval. The cache is used to
accelerate the determination of the final insert point to insert
spill/split for current LiveInterval.

Thanks,
Wei.

Ok then this patch LGTM.

I would apreciate it, if you could change current live interval to just be passed to the InsertAnalaysis queries instead of being a member in a follow-up commit.

This revision is now accepted and ready to land.May 11 2016, 3:09 PM
wmi added a comment.May 11 2016, 3:14 PM

Ok, I will do that. Thanks to you, Quentin and Eric for the helpful comments!

Wei.

MatzeB closed this revision.Jul 20 2016, 8:22 PM

This was committed in r269249 (I guess phab didn't pickup the differential revision line because it had a dot at the end)