This is an archive of the discontinued LLVM Phabricator instance.

[AssumeBundles] Adapt RecursivelyDeleteTriviallyDeadInstructions and depending passes.
AbandonedPublic

Authored by Tyker on Apr 8 2020, 7:53 AM.

Details

Diff Detail

Event Timeline

Tyker created this revision.Apr 8 2020, 7:53 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert added inline comments.Apr 8 2020, 8:55 AM
llvm/lib/Transforms/IPO/Attributor.cpp
1102

This looks suspicious. Are we sure we don't invalidate an element in here and need the tracking behavior?

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
139 ↗(On Diff #256026)

Could you explain this check? When would a call return a nullptr of an arg operand? isn't there an assertion against that?

210 ↗(On Diff #256026)

This doesn't sound right. null operands will explode everywhere, how could they happen?

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
220

I guess this is an NFC rewrite? We should split it into a separate commit though, only this file, creating the members.

Tyker updated this revision to Diff 256055.Apr 8 2020, 10:03 AM
Tyker marked an inline comment as done.

Sorry i messed up will uploading the diff.
this diff should be fine.

jdoerfert added inline comments.Apr 8 2020, 2:51 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1192

Are we sure the instruction could not have been deleted at this point? If not we need to check for null in the cast.


I think it is reasonable to assume all instructions are in the same function. We should query AC before the loop.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
879

Unsure if it helps but one could add comments to the nullptr here and elsewhere: /* MSSAU */

llvm/test/Transforms/Attributor/ArgumentPromotion/byval-2.ll
37 ↗(On Diff #256055)

Note: We really have to teach the Attributor to prune these. I'll get to it or make it a task for one of the GSoC students.

jdoerfert added inline comments.Apr 8 2020, 9:07 PM
llvm/test/Transforms/Attributor/ArgumentPromotion/byval-2.ll
3 ↗(On Diff #256055)

When you rebase you'll get a conflict. I recommend using the new file and adding --enable-knowledge-retention to some or all of the 4 run configurations. check lines are generated :)

Tyker updated this revision to Diff 256278.Apr 9 2020, 6:38 AM
Tyker marked 5 inline comments as done.

addressed comments.

Note: this patch depend on D77402 and D76149 that should be reviewed first.

Tyker added inline comments.Apr 9 2020, 6:40 AM
llvm/test/Transforms/Attributor/ArgumentPromotion/byval-2.ll
3 ↗(On Diff #256055)

thanks for the heads up.

37 ↗(On Diff #256055)

by prune you mean removing redundant knowledge ?

Tyker updated this revision to Diff 262657.May 7 2020, 8:06 AM

rebased

I think we need to run some of the affected passes with knowledge preservation to get more test coverage. Also two more comments below.

llvm/lib/Transforms/IPO/Attributor.cpp
1192

I'm unsure why I thought all dead instructions are in the same function. I somehow fear we need to sort them by function. Maybe we can go ahead with the patch without this change first?

llvm/unittests/Transforms/Utils/LocalTest.cpp
39

Commit this as test improvements.

What is the status on this one? Are there other AssumeBundle patches pending or missing?

Tyker added a comment.Jul 8 2020, 10:49 AM

What is the status on this one? Are there other AssumeBundle patches pending or missing?

since the creation of the patch many patches on assume bundles have landed and I don't think this patch is needed any more

Tyker abandoned this revision.Aug 21 2022, 10:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2022, 10:38 AM
Herald added a subscriber: ormris. · View Herald Transcript