[PATCH] D152431: [Inliner] Handle convergence control when inlining a call

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked an inline comment as done.
sameerds added a comment.

In D152431#4433182 <https://reviews.llvm.org/D152431#4433182>, @nhaehnle wrote:

> The proliferation of the ConvergenceControlUsage makes me a bit uncomfortable. It adds the overhead of an additional iteration over entire functions every time a function is inlined or perhaps the inlining is even attempted. And the only thing we get in exchange is a safety check against mixing controlled and uncontrolled convergent operations. Is it really worth it? I'd expect that in general, a compiler either always emits controlled or always emits uncontrolled operations, which means this isn't actually a scenario we need to worry about too much.

Agreed. I've removed that analysis and replaced it with a simple check that requires a token operand when the called function contains the entry intrinsic. This is clearly incomplete, but a useful check to have. See comments in InlineFunction.cpp and also in the review description.

Just a thought: Eventually, if we do want to disallow inlining in the presence of both kinds of convergence, we could repurpose the `convergent` attribute by first making all functions convergent by default. Then we can say that uncontrolled convergent calls are only allowed in a function with the "legacy" `convergent` attribute. The same function cannot contain controlled convergent calls, including the `entry` intrinsic.



================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:2303-2312
+  if (ConvergenceControlToken) {
+    auto *I = FirstNewBlock->getFirstNonPHI();
+    if (auto *IntrinsicCall = dyn_cast<IntrinsicInst>(I)) {
+      if (IntrinsicCall->getIntrinsicID() ==
+          Intrinsic::experimental_convergence_entry) {
+        IntrinsicCall->replaceAllUsesWith(ConvergenceControlToken);
+        IntrinsicCall->eraseFromParent();
----------------
nhaehnle wrote:
> This looks like it's making an assumption that the `convergence.entry` is the very first instruction. Do we really want to enforce that? Definitely needs a verifier check in that case...
Yes. We enforce it in D147116.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152431