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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
181 | Nit: if (!Bundle) continue to reduce indention or if (... Bundle = ...) | |
191 | [Is this C++14?] | |
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
80 | 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. |
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 ?
llvm/lib/Analysis/AssumeBundleQueries.cpp | ||
---|---|---|
191 | yes, | |
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
80 | personally i prefer not having a constructor (or function) taking too many argument as i find it harder to read and write. |
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 | ||
---|---|---|
80 | 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. |
We should later also look at the value itself. It is a position that can take attributes it might have one attached.