Page MenuHomePhabricator

[OpenMP][FIX] Be more deliberate about invalidating the AAKernelInfo state
ClosedPublic

Authored by jdoerfert on Sep 8 2021, 3:01 PM.

Details

Summary

This patch fixes a problem when the AAKernelInfo state was invalidated,
e.g., due to optnone for a kernel, but not all parts indicated the
invalidation properly. We further eliminate most full state
invalidations as they should never me necessary.

Diff Detail

Event Timeline

jdoerfert created this revision.Sep 8 2021, 3:01 PM
jdoerfert requested review of this revision.Sep 8 2021, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2021, 3:01 PM
tianshilei1992 added inline comments.Sep 10 2021, 9:50 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
631–632

format

3045

Don't you expect anything else between in the future?

3349

From Attributor's perspective, they behave same, don't they?

3902–3903

IIRC, ReachedUnknownParallelRegions will be invalidated on insertion. With this change, this AA indicates optimistic fixed point?

jdoerfert added inline comments.Sep 10 2021, 10:01 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3045

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.

3349

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.

3902–3903

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.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3045

If we don't expect anything (any other manipulation) in the future between line 3006 and 3008, the change here is fine.

tianshilei1992 accepted this revision.Sep 10 2021, 10:48 AM

Sorry forget to accept the patch. Please fix the format before landing it.

This revision is now accepted and ready to land.Sep 10 2021, 10:48 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3902–3903

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.

jdoerfert added inline comments.Sep 10 2021, 11:11 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3902–3903

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.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3902–3903

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?

jdoerfert added inline comments.Sep 10 2021, 11:29 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3902–3903

A pessimistic fixpoint on AAKernelInfo should (almost) never be necessary. This patch removes (almost) all occurrences. A pessimistic fixpoint on AAKernelInfo will invalidate all information in the state, basically reverting everything to the previously known state. Since we track various things deduced in different ways, there is basically no good reason to invalidate everything at once. One could argue we should not track everything in a single state but that is a different discussion.

In your example, if X is invalidated it also invalidates reaching kernels. So Y will realize reaching kernels of X are unusable and deal with it accordingly (probably give up). However, the fact that this is a task runtime call did not impact the reaching kernels so there was no reason to invalidate it in the first place.

This doesn't seem to apply against main for me, please could someone more familiar with the patch rebase it?

This revision was landed with ongoing or failed builds.Wed, Sep 22, 10:05 PM
This revision was automatically updated to reflect the committed changes.