This is an archive of the discontinued LLVM Phabricator instance.

[IROutliner] Ensure instructions at end of candidate are excluded
ClosedPublic

Authored by AndrewLitteken on Jun 11 2021, 12:00 PM.

Details

Summary

Occasionally instructions are between the last instruction in a region, and the following instruction as identified by the Candidate. This adds an extra check right before splitting a candidate that excludes the region from being split/checked for outlining to remove errors.

Tests Changed:
Tranforms/IROutliner/outlining-extra-bitcasts.ll

Diff Detail

Event Timeline

AndrewLitteken requested review of this revision.Jun 11 2021, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2021, 12:00 PM
ormris removed a subscriber: ormris.Jun 11 2021, 2:49 PM
hiraditya added inline comments.Jun 13 2021, 12:47 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
1699

Do we need to split when the region is going to be ignored?

AndrewLitteken added inline comments.Jun 13 2021, 3:01 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
1699

There's a chance that when the region is split, extra instructions are added changing the possibility of outlining, and it is no longer viable to be split. It might be better to have this function return a boolean if it has been split, or has not been, then continue based on that result.

Update splitCandidates to escape early if the region cannot be extracted, and checking the boolean CandidateSplit for the region rather than using IgnoreRegion,

Can you add context to the diff?

Adding context to the diff.

paquette added inline comments.Jul 1 2021, 12:51 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
136

Move EndInst above and use it here?

Also, this could use a comment.

1699

Could use a comment explaining why this wouldn't happen.

llvm/test/Transforms/IROutliner/outlining-bitcasts.ll
40

Why did these lines disappear?

53
53

Sounds like "Outlining only occurs in this case due to the lack of a cost model" should be a FIXME?

AndrewLitteken added inline comments.Jul 2 2021, 7:27 AM
llvm/test/Transforms/IROutliner/outlining-bitcasts.ll
40

They move down to the bottom of the test, I think this was a function of the test generation script.

53

It's not really a fix me, the no cost command line argument is here to test these sorts of cases, so that it's not dependent on the way that the cost of outlining is calculated, rather how we define similarity. I can make this more clear.

Adding comments and fixes.

AndrewLitteken edited the summary of this revision. (Show Details)

Updating based on feedback, and adding extra comments.

paquette accepted this revision.Aug 25 2021, 1:36 PM

LGTM with nits

llvm/lib/Transforms/IPO/IROutliner.cpp
139

run-on sentence?

147

message should reflect the check

1700

run-on sentence?

llvm/test/Transforms/IROutliner/outlining-bitcasts.ll
53
This revision is now accepted and ready to land.Aug 25 2021, 1:36 PM

updating for nits.

This revision was landed with ongoing or failed builds.Aug 30 2021, 9:31 AM
This revision was automatically updated to reflect the committed changes.