[PATCH] D23738: [PM] Extend the explicit 'invalidate' method API on analysis results to accept an Invalidator that allows them to invalidate themselves if their dependencies are in turn invalidated.

[Justin Lebar via llvm-commits]

jlebar added inline comments.


================
Comment at: include/llvm/IR/PassManager.h:610
+      IMapI->second = Result.invalidate(IR, PA, Inv);
+    }
+
----------------
WRT the names:

I agree "invalidate" isn't a great name, because

  if (!my_pass->invalidate(...)) {
    // my_pass was not invalidated!
  }

`checkIfInvalidated`?  `onIRChange`?  `notifyIRChanged`?  I like the first one because the bool return value sort of makes sense.

This would also change the comment above -- we're not really "trying" to invalidate the result.  More informing it that something has changed, and giving it the option to mark itself as invalid.

I still don't really understand what we're supposed to do inside the `invalidate` function, so I'm not sure about the `Invalidator` class name.  The only example I have is

    bool TestIndirectFunctionAnalysis::invalidate(Function &F, const PreservedAnalyses &PA,
                    FunctionAnalysisManager::Invalidator &Inv) {
      return !PA.preserved<TestIndirectFunctionAnalysis>() ||
             Inv.invalidate<TestFunctionAnalysis>(F, PA);
    }

It naively seems to me that it would be better to structure things so that we never run this function if PA.preserved<TestIndirectFunctionAnalysis>().  The Invalidator could ensure this.  Then our function could be

  bool checkIfInvalidated(...) {
    return Inv.checkIfInvalidated<Dep1>() || Inv.checkIfInvalidated<Dep2>();
  }

which actually makes sense to me.  I think ideally Inv.checkIfInvalidated would take zero parameters, since it's just boilerplate, but maybe it should still take the Function/Module/whatever.  We'd at least be able to get rid of the PreservedAnalyses argument, I think?


https://reviews.llvm.org/D23738