[PATCH] D27198: [PM] Introduce the facilities for registering cross-IR-unit dependencies that require deferred invalidation.
Justin Lebar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 15 15:39:37 PST 2016
jlebar added a comment.
This looks good to me, and I'm basically ready to approve the patch, but there are a few new non-comment questions buried in here that I'd like to get resolved first.
================
Comment at: include/llvm/IR/PassManager.h:675
void invalidate(IRUnitT &IR, const PreservedAnalyses &PA) {
- // Short circuit for common cases of all analyses being preserved.
- if (PA.areAllPreserved() || PA.preserved<AllAnalysesOn<IRUnitT>>())
+ // We can only use one short circuit because even if another set is
+ // preserved there might be abandoned analyses within that set which still
----------------
chandlerc wrote:
> jlebar wrote:
> > It's not clear what this is contrasting with (without the diff available :).
> Reworded to hopefully make this more clear.
With your latest change to the PA interface, can we revert this change and instead do
if (PA.areAllPreserved() || PA.allAnalysesOnSetPreserved<AllAnalysesOn<IRUnitT>>())
?
================
Comment at: include/llvm/IR/PassManager.h:72
+/// A transformation can mark that it is preserving the CFG of a function and
+/// then analyses can check for this rather than having te fully enumerate
+/// every analysis preserved.
----------------
te
================
Comment at: include/llvm/IR/PassManager.h:73
+/// then analyses can check for this rather than having te fully enumerate
+/// every analysis preserved.
+struct alignas(8) AnalysisSetKey {};
----------------
I know what you mean, but the "rather than" part doesn't make sense -- *analyses* would never have to enumerate every analysis that's preserved. That's the job of the *transformations*.
================
Comment at: include/llvm/IR/PassManager.h:86
+/// This class also provides the ability to mark abstract *sets* of analyses as
+/// preserved. These sets allow passes to preserve broad aspects of the IR such
+/// as its CFG and analyses to opt in to that being sufficient without the
----------------
to indicate that they preserve
================
Comment at: include/llvm/IR/PassManager.h:87
+/// preserved. These sets allow passes to preserve broad aspects of the IR such
+/// as its CFG and analyses to opt in to that being sufficient without the
+/// passes having to fully enumerate such analyses.
----------------
set off "such as its CFG" with parens
================
Comment at: include/llvm/IR/PassManager.h:90
+///
+/// Finally, this class can represent "abandoning" an analysis which marks it
+/// as not-preserved even if it would be covered by some abstract set of
----------------
", which"
================
Comment at: include/llvm/IR/PassManager.h:94
+///
+/// The other aspect of this API supports analyses querying whether they have
+/// been preserved. Because analyses are expected to typically be part of sets,
----------------
First sentence could be clarified / simplified:
Given a PreservedAnalyses object built up by a transformation, an analysis will typically want to figure out whether it is preserved.
================
Comment at: include/llvm/IR/PassManager.h:95
+/// The other aspect of this API supports analyses querying whether they have
+/// been preserved. Because analyses are expected to typically be part of sets,
+/// the query API optimizes for this. Analyses can build a checker object and
----------------
s/are expected to typically be part of sets/are usually covered by one or more sets/
================
Comment at: include/llvm/IR/PassManager.h:96
+/// been preserved. Because analyses are expected to typically be part of sets,
+/// the query API optimizes for this. Analyses can build a checker object and
+/// then query it for themselves directly and for any sets of interest. This
----------------
"can" suggests that they have an option, but it's kind of the only choice.
================
Comment at: include/llvm/IR/PassManager.h:101
+/// preserved in order to cover its requirements, and this can be expressed
+/// naturally with `&&` in the query API:
+///
----------------
Suggest simplifying this whole para. Just introduce the idea and give the example.
> Given a PreservedAnalyses object, an analysis will typically want to figure out whether it is preserved. In the example below, MyAnalysisType is preserved if it's not abandoned, and (a) it's explicitly marked as preserved, (b), the set AllAnalysesOn<MyIRUnit> is preserved, or (c) both AnalysisSetA and AnalysisSetB are preserved.
================
Comment at: include/llvm/IR/PassManager.h:113
+/// This API directly supports the relationship between preserved sets and
+/// their being overriden by a specific "abandoned" analysis.
+///
----------------
Not sure this para is necessary with the rewrite above.
================
Comment at: include/llvm/IR/PassManager.h:117
+/// preserved. This checks if there could possibly be an abandoned analysis
+/// that would not be preserved even if the provided set is preserved.
class PreservedAnalyses {
----------------
Would suggest making this active, like the suggestion above:
> You can also ask a PreservedAnalyses object whether all analyses in a particular set are preserved. If *any* analyses have been abandoned, this *always* returns false, because PreservedAnalyses does not have a priori knowledge of which analyses are in which sets.
Alternatively, maybe this isn't necessary to include in this comment at all; it's kind of an edge case, and we don't have to enumerate the whole API here.
================
Comment at: include/llvm/IR/PassManager.h:133
- /// \brief Mark an abstract ID as preserved, adding it to the set.
+ /// Mark an abstract analysis ID as preserved.
void preserve(AnalysisKey *ID) {
----------------
Maybe
> Mark a particular analysis as preserved, given a pointer to its AnalysisKey.
or something. The current way of distinguishing between this and the one above -- "a particular analysis" versus "an abstract analysis ID" -- is not facile.
Same below.
================
Comment at: include/llvm/IR/PassManager.h:157
+ /// Mark a particular analysis as abandoned, removing it from the preserved
+ /// set even if covered by some other set or previously explicitly marked as
+ /// preserved.
----------------
"if it's covered" "was previously marked as preserved".
================
Comment at: include/llvm/IR/PassManager.h:164
+
+ /// Mark a particular analysis ID as abandoned, removing it from the
+ /// preserved set even if covered by some other set.
----------------
Again here, we are still marking the analysis -- not an ID -- as abandoned. The difference is in what we're *given*, not really what we *do*.
================
Comment at: include/llvm/IR/PassManager.h:176
+ // to be preserved.
+ NotPreservedAnalysisIDs.insert(ID);
}
----------------
I am not sure either of these comments in the body are necessary, personally. They seem to repeat the code and the function-level comment.
================
Comment at: include/llvm/IR/PassManager.h:190
}
- for (auto ID : PreservedAnalysisIDs)
- if (!Arg.PreservedAnalysisIDs.count(ID))
- PreservedAnalysisIDs.erase(ID);
+ // The intersection requires the *union* of the explicitly not preserved
+ // IDs and the *intersection* of the preserved IDs.
----------------
Nit, "not-preserved".
================
Comment at: include/llvm/IR/PassManager.h:227
+ /// We need to know whether the analysis has been explicitly abandoned even
+ /// when querying the preserved sets in order to skip them.
+ class PreservedAnalysisChecker {
----------------
Perhaps this sentence should live on the constructor:
> We take an AnalysisKey in our constructor because we need to know ...
I think maybe you had this comment here because you wanted to clarify what `preserved` and `preservedSet` return without writing repetitive comments? I think it's probably worth having brief comments there:
> /// Returns true if our analysis was not abandoned and (a) the analysis was explicitly preserved, or (b) all analyses were preserved.
> /// Returns true if our analysis was not abandoned and (a) the set was explicitly preserved, or (b) all analyses were preserved.
================
Comment at: include/llvm/IR/PassManager.h:253
+ ///
+ /// Returns a checker that can in turn be queried both for the overal
+ /// preservation and any relevant analysis sets.
----------------
s/in turn//
================
Comment at: include/llvm/IR/PassManager.h:253
+ ///
+ /// Returns a checker that can in turn be queried both for the overal
+ /// preservation and any relevant analysis sets.
----------------
jlebar wrote:
> s/in turn//
overal
================
Comment at: include/llvm/IR/PassManager.h:254
+ /// Returns a checker that can in turn be queried both for the overal
+ /// preservation and any relevant analysis sets.
+ template <typename AnalysisT> PreservedAnalysisChecker getChecker() const {
----------------
The prep phrase starting with "for" doesn't make much sense. Maybe say:
> You can use this object to query whether an analysis was preserved. See the example in the comment on PreservedAnalysis.
================
Comment at: include/llvm/IR/PassManager.h:262
+ ///
+ /// Returns a checker that can in turn be queried both for the overal
+ /// preservation and any relevant analysis sets.
----------------
Same comments here.
================
Comment at: include/llvm/IR/PassManager.h:268
- /// \brief Test whether all passes are preserved.
+ /// Test whether all analyses are preserved.
///
----------------
Suggest being explicit "preserved (and none are abandoned)."
================
Comment at: include/llvm/IR/PassManager.h:270
///
/// This is used primarily to optimize for the case of no changes which will
/// common in many scenarios.
----------------
Suggest something like
> This lets analyses optimize for the common case where a transformation made no changes to the IR.
================
Comment at: include/llvm/IR/PassManager.h:280
+ /// This is only true when no analyses has been explicitly abandoned and thus
+ /// sets which seem to cover it do not.
+ template <typename AnalysisSetT> bool allAnalysesInSetPreserved() const {
----------------
Suggest deleting starting with "and" -- it confuses more than it helps.
================
Comment at: include/llvm/IR/PassManager.h:288
+ /// This is only true when no analyses has been explicitly abandoned and thus
+ /// sets which seem to cover it do not.
+ bool allAnalysesInSetPreserved(AnalysisSetKey *SetID) const {
----------------
ibid.
================
Comment at: include/llvm/IR/PassManager.h:297
- SmallPtrSet<AnalysisKey *, 2> PreservedAnalysisIDs;
+ /// The set of preserved analyses.
+ ///
----------------
Suggest
> The analyses and analysis sets that are preserved.
>
> Invariant: A given AnalysisKey is never in both PreservedIDs set and NotPreservedAnalysisIDs.
================
Comment at: include/llvm/IR/PassManager.h:300
+ /// Unless covered directly or via some set here, analyses are assumed to not
+ /// be preserved.
+ SmallPtrSet<void *, 2> PreservedIDs;
----------------
I think this is pretty obvious, not sure it's necessary to say.
================
Comment at: include/llvm/IR/PassManager.h:307
+ /// analysis not preserved. It always wins over the above set and should not
+ /// include any synthetic set IDs such as the "all" ID.
+ SmallPtrSet<AnalysisKey *, 2> NotPreservedAnalysisIDs;
----------------
Now can remove
> and should not include any synthetic set IDs such as the "all" ID.
because type-safety. Suggest rewriting para to
> An analysis cannot be in both PreservedIDs and NotPreservedAnalysisIDs. If an analysis is covered by a set in PreservedIDs but is in NotPreservedAnalysisIDs, we consider it not-preserved. That is, NotPreservedAnalysisIDs always "wins" over analysis sets in PreservedIDs.
================
Comment at: include/llvm/IR/PassManager.h:560
+ ///
+ /// This is sadly less efficient than the above routine which leverages the
+ /// type parameter to avoid the type erasure overhead, but in some cases
----------------
", which" and then start a new sentence at "but".
There is a rule for the comma here: If you have a "which/that/who" phrase that is not "narrowing", you almost always offset the phrase with commas. A "narrowing" "which/that/who" phrase restricts the meaning of the phrase before: "My coworker who uses ed is out sick." (No commas, I have more than one coworker.) If the phrase is not narrowing, it gets a comma: "My dad, who is a programmer, lives in CA." (Commas; I have only one dad.)
Like everything in English, it depends, but this one is relatively safe.
================
Comment at: include/llvm/IR/PassManager.h:562
+ /// type parameter to avoid the type erasure overhead, but in some cases
+ /// the caller needed to do type erasure themselves.
+ bool invalidate(AnalysisKey *ID, IRUnitT &IR, const PreservedAnalyses &PA) {
----------------
s/needed/needs/?
Although, is the caller really erasing the types itself? Maybe you can just delete starting with "but".
================
Comment at: include/llvm/IR/PassManager.h:570
+
+ /// Helper to implemente the invalidate methods above, see their
+ /// documentation for the detailed interface. This implementation is
----------------
implemente
================
Comment at: include/llvm/IR/PassManager.h:573
+ /// factored to allow common code to be used whether we can compute
+ /// a concrete result type or we need to use the type erased concept type.
+ template <typename ResultT = ResultConceptT>
----------------
Not sure we need the sentence starting with "This implementation" -- it seems pretty clear.
================
Comment at: lib/Analysis/CGSCCPassManager.cpp:117
+ // Directly check if the relevant set is preserved so we can short circuit
+ // invalidating SCCs below without re-querying the preserved set.
+ bool AreSCCAnalysesPreserved =
----------------
Suggest s/without.*//
================
Comment at: lib/Analysis/LoopPassManager.cpp:37
+ auto PAC = PA.getChecker<LoopAnalysisManagerFunctionProxy>();
+ if (!PAC.preserved())
InnerAM->clear();
----------------
You don't want to check an AllAnalysesOn here too?
================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:828
+///
+/// FIXME: Currently this doesn't also depend on a function analysis and if it
+/// did we would fail to invalidate it correctly.
----------------
", and" (comma separates independent clauses)
https://reviews.llvm.org/D27198
More information about the llvm-commits
mailing list