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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3012
     if (!changeToSPMDMode(A))
-      buildCustomStateMachine(A);
+      return buildCustomStateMachine(A);
 
----------------
tianshilei1992 wrote:
> Don't you expect anything else between in the future?
I do not understand the question. Generally, if this is not proper in the future we can change it.
For now, we should return the right thing, that is unchanged if we did not do anything.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3310
     if (DisableOpenMPOptStateMachineRewrite)
-      return indicatePessimisticFixpoint();
+      return ChangeStatus::UNCHANGED;
 
----------------
tianshilei1992 wrote:
> From `Attributor`'s perspective, they behave same, don't they?
Not if we look at AAKernelInfo as part of the manifest of another AA, which we might already do.
So folding might not be performed because AAKernelInfo is invalid while we only wanted to disable the state machine customization.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3848
       SPMDCompatibilityTracker.insert(&CB);
       ReachedUnknownParallelRegions.insert(&CB);
+      break;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109468