This is an archive of the discontinued LLVM Phabricator instance.

[AssumeBundles] Prevent generation of some redundant assumes
ClosedPublic

Authored by Tyker on Apr 13 2020, 4:49 AM.

Details

Summary

with this patch the assume salvageKnowledge will not generate assume if all knowledge is already available in an assume with valid context. assume bulider can also in some cases update an existing assume with better information.

Diff Detail

Event Timeline

Tyker created this revision.Apr 13 2020, 4:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2020, 4:49 AM

I think this is fine, if it is not too costly to run these checks. I am not sure if it is worth it at the end of the day given that we will still need a pass to merge and remove assumes as these opportunities become available. I mean, after running the Attributor the information might be tied to arguments which we makes the assume obsolete. After inlining we might have multiple assumes with overlapping information we want to merge. Etc.

llvm/lib/Analysis/AssumeBundleQueries.cpp
180

Nit: if (!Bundle) continue to reduce indention or if (... Bundle = ...)

190

[Is this C++14?]

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
77

It should be nullptr or better initialized by the constructor. Same for the other members. There should be a constructor to initialize all of this and nullptrs otherwise.

Tyker updated this revision to Diff 257634.EditedApr 15 2020, 2:12 AM
Tyker marked 3 inline comments as done.

addressed comments.

i ran the compile-time benchmark, the previous version of the patch had some significant slowdown but disabling tryToPreserveWithoutAddingAssume when no AssumptionCache is available brought the difference within noise levels.
the measurement was between current master and master + all the AssumeBundles patch not yet committed with knowledge retention enabled.

you are probably right that doing this on the flight isn't enought and we still need a pass to prune and regroup assume bundles.
this seems like it should be run quite often since many passes can affect assume bundles.
any thought ?

Tyker added inline comments.Apr 15 2020, 4:43 AM
llvm/lib/Analysis/AssumeBundleQueries.cpp
190

yes,
C++14 is the default build mode.
if you want me to make it C++11 compatible i can.

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
77

personally i prefer not having a constructor (or function) taking too many argument as i find it harder to read and write.
but AC should definitely be default initialized to nullptr.

i ran the compile-time benchmark, the previous version of the patch had some significant slowdown but disabling tryToPreserveWithoutAddingAssume when no AssumptionCache is available brought the difference within noise levels.
the measurement was between current master and master + all the AssumeBundles patch not yet committed with knowledge retention enabled.

I think we can go ahead. The pass might reuse this logic. I'll give it another look later though.

you are probably right that doing this on the flight isn't enought and we still need a pass to prune and regroup assume bundles.
this seems like it should be run quite often since many passes can affect assume bundles.
any thought ?

I don't think it will be too bad. Given this patch, trivial assumes are not placed so we get some on-the-fly pruning.
We need to run tests but I can imagine to run the new pass after things like the inliner and Attributor. The first creates opportunities
to merge/prune assumes in the new function, the second manifests information in other places reducing the need for assumes. At the end
of the day the number of times we run it might not matter if it is fast. The OpenMPOpt pass looks for runtime calls and where they are to
determine if any work should be done. That is, the pass is a no-op if no llvm.assume is present. The pass can ignore all functions without
llvm.assume. All in all, it scales with the assumes and I hope the queries we run are not really expensive (e.g., DomTree queries).
I'm not sure if we want this as part of the Attributor or not.

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
77

This looks good now. I hope this is OK for you as well.

FWIW, I want to avoid the pattern that you create via constructor, then initialize some members later on via a second scheme. That is complicated and error prone, e.g., it is easy to forget the second step.

jdoerfert accepted this revision.May 7 2020, 9:40 AM

LGTM. See the two comments below

llvm/lib/Analysis/AssumeBundleQueries.cpp
171

We should later also look at the value itself. It is a position that can take attributes it might have one attached.

180

?

This revision is now accepted and ready to land.May 7 2020, 9:40 AM
This revision was automatically updated to reflect the committed changes.