[Openmp-commits] [PATCH] D109468: [OpenMP][FIX] Be more deliberate about invalidating the AAKernelInfo state

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3848
       SPMDCompatibilityTracker.insert(&CB);
       ReachedUnknownParallelRegions.insert(&CB);
+      break;
----------------
jdoerfert wrote:
> tianshilei1992 wrote:
> > jdoerfert wrote:
> > > tianshilei1992 wrote:
> > > > IIRC, `ReachedUnknownParallelRegions` will be invalidated on insertion. With this change, this AA indicates optimistic fixed point?
> > > Yes. But optimistic fixpoint != everything is optimistic. It will move the known state to the assumed state. Given that we already invalidated ReachedUnknownParallelRegions before, it won't "un-invalidate" it, ever.
> > I'm fine as long as this is consistent. But in other hand, I feel it might be better to invalid the AA as well if it is using another state that has already been invalidated.
> I don't follow. If an AA (X) uses another one (Y) and Y has an invalid state, X should not use the state of Y, ever. That is not a question nor is it changed here in any way. What is changed here is the fact that we do not need to give up on all information provided by AAKernelInfoCallSite if we see an __kmpc_omp_task call. We cannot argue SPMD and we cannot know all unknown parallel regions but we can certainly still know ReachingKernels (among other things). Indicating a pessimistic fixpoint here will give up on all information including stuff that we can use without problem.
`indicatePessimisticFixpoint` invalidates the AA (X), which is `AAKernelInfoCallSite` here, as well. When another AA (Y) uses its member, such as `ReachingKernels`, if we only check the state of Y, it's gonna break. If we directly check the member instead, then we are good. That's what I mean by "consistent".

But yeah, I agree now that we could use partial information. However, if it is fixed point, no matter optimistic or pessimistic, it will not be updated, right? What if the partial information we want to use gets updated? Is it possible?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109468/new/

https://reviews.llvm.org/D109468