[PATCH] D106859: [Sink] allow sinking convergent operations across uniform branches

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:351-354
+  if (ContainsIrreducible)
+    return true;
+  if (!DA)
+    return false;
----------------
foad wrote:
> I think it would be less confusing to put the `!DA` check before the `ContainsIrreducible` check. I realise you have initialised ContainsIrreducible to false so that this still works, even for an "empty" analysis with no DA or LegacyDA, but that seems like a lie since we don't actually know whether the function constains irreducible regions or not.
> 
> I would hope for a structure like:
> ```
> if (LegacyDA)
>   return (something based on LegacyDA);
> if (DA)
>   return (something based on DA);
> return false;
> ```
> 
> Same for the other functions below.
Addressed with a new enum that always has a consistent value. It has other uses too, so it is checked first.


================
Comment at: llvm/lib/Transforms/Scalar/Sink.cpp:246
   auto &AA = AM.getResult<AAManager>(F);
+  auto &DI = AM.getResult<DivergenceAnalysis>(F);
 
----------------
foad wrote:
> It seems odd that you explicitly choose the new DA here but the legacy DA in SinkingLegacyPass. I know they both have "legacy" in their name but that doesn't seem like a good reason. SinkingLegacyPass is just a wrapper for use with the legacy pass manager, but I don't see why it should necessarily use the legacy DA.
> 
> Or is there some problem like the legacy DA doesn't work with the new PM, which has forced you to do this?
The legacy DA was never ported to the new pass manager. It can only be invoked from the old pass manager, where it is the default DA. See the RUN lines in the updated lit test to see all the valid combinations.


================
Comment at: llvm/test/Transforms/Sink/convergent.ll:6
+; RUN: opt -enable-new-pm=1 -sink -gpuda-assume-always-divergent -S < %s | FileCheck %s -check-prefixes=CHECK,DIV
+; RUN: opt -enable-new-pm=0 -sink -use-gpu-divergence-analysis -gpuda-assume-always-divergent -S < %s | FileCheck %s -check-prefixes=CHECK,DIV
 
----------------
The earlier attempt had copied the test into two versions, one with the default target and one with AMDGPU. But this is not scalable with more changes in the pipeline that have lots of other tests. Instead, the new option forces the new DA to report "divergent" for everything, which allows us to test convergent operations even on the default target. The tests can then run in every build, ensuring good coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106859