This is an archive of the discontinued LLVM Phabricator instance.

[Inliner] Handle `mustprogress` functions
ClosedPublic

Authored by atmnpatel on Sep 7 2020, 6:53 PM.

Details

Summary

When inlining mustprogress functions, if the caller or the callee has
the attribute, we drop the function attribute. The loops that have the
llvm.loop.mustprogress metadata keep their metadata. We do not need to
add new loop metadata to inlined functions because the patch in D86841
already adds the relevant loop metadata in all of the necessary places.

Diff Detail

Event Timeline

atmnpatel created this revision.Sep 7 2020, 6:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2020, 6:53 PM
atmnpatel requested review of this revision.Sep 7 2020, 6:53 PM

Add a test case with multiple loops in the caller and callee please. also nested ones. And one with the callee loop not having the metadata so we see it won't after.
And a test with both caller and callee having the attribute so no metadata is added.

llvm/lib/Transforms/Utils/InlineFunction.cpp
2316 ↗(On Diff #290391)

Don't walk the blocks but the loops. LoopInfo exposes only top-level loops but you can use it as a graph or employ a worklist to get to the nested ones. I usually see the latter.

atmnpatel updated this revision to Diff 290626.Sep 8 2020, 7:24 PM

Added tests and changed the loop iteration method.

atmnpatel updated this revision to Diff 291481.Sep 13 2020, 5:52 PM

Flipped directions, now it gives the callee loops llvm.loop.mustprogress if a mustprogress function is being inlined.

atmnpatel retitled this revision from [Inliner] Apply llvm.loop.mustprogress to caller loops to [Inliner] Apply llvm.loop.mustprogress to callee loops if callee is `mustprogress`.Sep 13 2020, 5:53 PM
atmnpatel edited the summary of this revision. (Show Details)
nikic added a subscriber: nikic.Sep 25 2020, 12:46 PM

Assuming it's not part of some other patch, this should also add a LangRef entry for the llvm.loop.mustprogress metadata.

I also don't see where this avoids applying the metadata if the caller is mustprogress as well. Seems to work fine in the tests, but I don't understand why looking at the code...

atmnpatel updated this revision to Diff 294463.Sep 25 2020, 5:23 PM

Added LangRef definition, added missing test and corresponding logic.

atmnpatel updated this revision to Diff 294475.Sep 25 2020, 9:52 PM

Formatting changes.

Rebase to fix buildkite failure.

Please split the loop metadata and the inliner stuff. I think the metadata is needed in clang already, it should go in after the functionattr IR patch

atmnpatel updated this revision to Diff 295157.Sep 29 2020, 6:06 PM

Inliner changes now that the clang implementation in D86841 has changed. Can I remove the inliner nested loops and multiple loops tests now? They're not as relevant anymore.

atmnpatel retitled this revision from [Inliner] Apply llvm.loop.mustprogress to callee loops if callee is `mustprogress` to [Inliner] Handle `mustprogress` functions.Sep 29 2020, 6:45 PM
atmnpatel edited the summary of this revision. (Show Details)
atmnpatel updated this revision to Diff 295161.Sep 29 2020, 6:46 PM

Updated commit message.

jdoerfert accepted this revision.Sep 29 2020, 11:13 PM

LGTM

llvm/lib/Transforms/Utils/InlineFunction.cpp
63 ↗(On Diff #295161)

unrelated.

This revision is now accepted and ready to land.Sep 29 2020, 11:13 PM
atmnpatel updated this revision to Diff 297101.Oct 8 2020, 6:25 PM

Fixed nit.

This revision was landed with ongoing or failed builds.Nov 6 2020, 5:04 PM
This revision was automatically updated to reflect the committed changes.