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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:351-354
+  if (ContainsIrreducible)
+    return true;
+  if (!DA)
+    return false;
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/Sink.cpp:246
   auto &AA = AM.getResult<AAManager>(F);
+  auto &DI = AM.getResult<DivergenceAnalysis>(F);
 
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106859