[PATCH] D146018: [AMDGPU] Use UniformityAnalysis in AtomicOptimizer

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericUniformityImpl.h:1102
+  for (const CycleT *Cycle = CI.getCycle(DefBlock);
+       Cycle && !Cycle->contains(&ObservingBlock);
+       Cycle = Cycle->getParentCycle()) {
----------------
foad wrote:
> sameerds wrote:
> > Pierre-vh wrote:
> > > foad wrote:
> > > > 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))`?
> > > I'm not too familiar with CycleInfo so I'll leave this one to @sameerds
> > 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.
> > so this function is simply a lookup in a set
> 
> It's implemented as a linear search through a std::vector.
Ouch, I didn't notice that! We can do that as a separate change. I volunteer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146018