[PATCH] D155884: [Attributor][AMDGPU] Improve indirect call support in closed modules

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3254-3267
+  if (isClosedWorldModule() || (!isModulePass() && !FI.CalledViaMustTail)) {
+    bool IsIndirectlyCallable = !isClosedWorldModule() || !F.hasLocalLinkage();
+    for (const Use &U : F.uses()) {
       if (const auto *CB = dyn_cast<CallBase>(U.getUser()))
-        if (CB->isCallee(&U) && CB->isMustTailCall())
-          FI.CalledViaMustTail = true;
+        if (CB->isCallee(&U))
+          if (CB->isMustTailCall()) {
+            FI.CalledViaMustTail = true;
----------------
scott.linder wrote:
> I don't understand the context fully, but is the intention to have `&F` pushed into `IndirectlyCallableFunctions` repeatedly if there are repeated calls to it? And that earlier-ordered calls are pushed to it, but none are considered once any must-tail-call are reached? I would have expected the push to be pulled out of the loop, as those constraints don't sound meaningful to me. The `continue` case is also confusing to me, unless there is some side-effect in the loop I'm missing? The same question applies to the original code, where I would have expected a `break` after `CalledViaMustTail` is set to `true`, so I assume I am just missing something?
> 
> In addition to all the above confusion the condition on the outermost `if` also seems strange, as there are two separable cases, one of which depends on the other, but they are combined in a non-obvious way, namely around the starting value of `FI.CalledViaMustTail`.
> 
> I added a suggestion of what makes sense to me, but there are far too many things I'm confused about to have any confidence my suggestions are useful. Any help in understanding would be appreciated!
> I don't understand the context fully, but is the intention to have &F pushed into IndirectlyCallableFunctions repeatedly if there are repeated calls to it?

No. It always ends up in there once. I might have placed it in the wrong scope. I'll check your code.

> The continue case is also confusing to me, unless there is some side-effect in the loop I'm missing? The same question applies to the original code, where I would have expected a break after CalledViaMustTail is set to true, so I assume I am just missing something?

The original code seems to be missing a break.

> In addition to all the above confusion the condition on the outermost if also seems strange, as there are two separable cases, one of which depends on the other, but they are combined in a non-obvious way, namely around the starting value of FI.CalledViaMustTail.

The outer if has two conditions separated with a `||`, no?
If either is true, we want to look at users.
First is `isClosedWorldModule()`, second is ` (!isModulePass() && !FI.CalledViaMustTail)`.
Why do they depend on each other? Maybe the confusion is that we might search for must tail if we don't need to, which is a fair criticism. If we set closed world in the backend (for now), I'll clean this up.



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

https://reviews.llvm.org/D155884