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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Looks good overall.



================
Comment at: llvm/include/llvm/ADT/GenericUniformityImpl.h:466
-  /// \brief Whether \p Val is divergent when read in \p ObservingBlock.
+  /// \brief Whether \p Def is divergent when read in \p ObservingBlock.
   bool isTemporalDivergent(const BlockT &ObservingBlock,
-                           ConstValueRefT Val) const;
----------------
Was this declaration unused?


================
Comment at: llvm/include/llvm/ADT/GenericUniformityImpl.h:1102
+  for (const CycleT *Cycle = CI.getCycle(DefBlock);
+       Cycle && !Cycle->contains(&ObservingBlock);
+       Cycle = Cycle->getParentCycle()) {
----------------
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))`?


================
Comment at: llvm/lib/Analysis/UniformityAnalysis.cpp:87
+    return true;
+  const auto *UseInstr = cast<Instruction>(U.getUser());
+  if (const auto *DefInstr = dyn_cast<Instruction>(V))
----------------
I think this should go inside the "if" below, since Def being an Instruction implies that Use is also an Instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146018