This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Use UniformityAnalysis in AtomicOptimizer
ClosedPublic

Authored by Pierre-vh on Mar 14 2023, 12:53 AM.

Details

Summary

Adds & uses a new isDivergentUse API in UA.
UniformityAnalysis now requires CycleInfo as well as the new temporal divergence API can query it.


Original patch that adds isDivergentUse by @sameerds

The user of a temporally divergent value is marked as divergent in the
uniformity analysis. But the same user may also have been marked divergent for
other reasons, thus losing this information about temporal divergence. But some
clients need to specificly check for temporal divergence. This change restores
such an API, that already existed in DivergenceAnalysis.

Diff Detail

Event Timeline

Pierre-vh created this revision.Mar 14 2023, 12:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 12:53 AM
Pierre-vh requested review of this revision.Mar 14 2023, 12:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 12:53 AM
foad added a comment.Mar 14 2023, 2:34 AM

Looks good overall.

llvm/include/llvm/ADT/GenericUniformityImpl.h
466

Was this declaration unused?

1102

Nit: the Cycle->contains check seems like it could get expensive on pathological cases with thousands of nested loops. Would it be more efficient to walk up the cycle tree from getCycle(Def) to nearestCommonAncestor(getCycle(Def), getCycle(Use))?

llvm/lib/Analysis/UniformityAnalysis.cpp
87

I think this should go inside the "if" below, since Def being an Instruction implies that Use is also an Instruction.

Pierre-vh updated this revision to Diff 505001.Mar 14 2023, 2:52 AM
Pierre-vh marked 2 inline comments as done.

Comments

llvm/include/llvm/ADT/GenericUniformityImpl.h
466

Yes, it had no definition for UA beforehand.

1102

I'm not too familiar with CycleInfo so I'll leave this one to @sameerds

sameerds added inline comments.Mar 14 2023, 3:32 AM
llvm/include/llvm/ADT/GenericUniformityImpl.h
1102

Every cycle contains a list of all the blocks that it transitively contains, so this function is simply a lookup in a set. This is the same behaviour as LoopInfo.

But now that nested cycles got mentioned, I think this change should have another test with a def inside the inner of two nested cycles, and a use of that def outside the outer cycle.

foad added inline comments.Mar 14 2023, 4:31 AM
llvm/include/llvm/ADT/GenericUniformityImpl.h
1102

so this function is simply a lookup in a set

It's implemented as a linear search through a std::vector.

Pierre-vh updated this revision to Diff 505044.Mar 14 2023, 5:13 AM

Add nested cycle test + remove unneeded run line

sameerds added inline comments.Mar 14 2023, 8:06 AM
llvm/include/llvm/ADT/GenericUniformityImpl.h
1102

Ouch, I didn't notice that! We can do that as a separate change. I volunteer.

sameerds accepted this revision.Mar 14 2023, 11:05 PM

LGTM, but wait for @foad to comment.

This revision is now accepted and ready to land.Mar 14 2023, 11:05 PM
foad accepted this revision.Mar 15 2023, 12:09 AM
This revision was landed with ongoing or failed builds.Mar 15 2023, 1:40 AM
This revision was automatically updated to reflect the committed changes.