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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3250
 
   // In non-module runs we need to look at the call sites of a function to
   // determine if it is part of a must-tail call edge. This will influence what
----------------
Comment should describe the new case


================
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;
----------------
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!


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

https://reviews.llvm.org/D155884