[PATCH] D36850: [ThinLTO] Add norecurse function attribute propagation

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: clang/test/CodeGen/thinlto-funcattr-prop.ll:14
+
+; RUN: llvm-dis %t/b.bc -o - | FileCheck %s
+
----------------
tejohnson wrote:
> This is checking the summary generated by opt, not the result of the llvm-lto2 run.
Fixed.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:580
+    // If there are calls to unknown targets (e.g. indirect)
+    unsigned hasUnknownCall : 1;
+
----------------
tejohnson wrote:
> tejohnson wrote:
> > modimo wrote:
> > > tejohnson wrote:
> > > > Now that we have MayThrow, can we avoid a separate hasUnknownCall bool and just conservatively set MayThrow true in that case?
> > > hasUnknownCall is used for norecurse and other future flags as well to stop propagation.
> > Ah that makes sense.
> nit, maybe change this to hasIndirectCall which I think is more specific?
My thinking is that the flag is a catch-all for blocking propagation and could conceivably be set for other reasons. It also matches the existing usage in FunctionAttrs.cpp for local propagation which also sets this for functions that are `OptNone`.


================
Comment at: llvm/include/llvm/LTO/LTO.h:26
 #include "llvm/Support/thread.h"
+#include "llvm/Transforms/IPO/FunctionAttrs.h"
 #include "llvm/Transforms/IPO/FunctionImport.h"
----------------
tejohnson wrote:
> Is this needed?
Yeah, `thinLTOPropagateFunctionAttrs` resides in FunctionAttrs.h and `runThinLTO` calls it to propagate.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:379
       } else {
+        HasUnknownCall = true;
         // Skip inline assembly calls.
----------------
tejohnson wrote:
> Should this be moved below the following checks for inline asm and direct calls? (Not sure what the direct calls case is given that we handle direct calls to "known functions" above though).
> 
> If it should stay where it is and treat the below cases as unknown, probably should add tests for them.
Any call that isn't emitted to the summary CallGraphEdges is a hole in propagation knowledge.

Direct calls case is from https://reviews.llvm.org/D40056 which is handling:
```
    ; Test calls that aren't handled either as direct or indirect.
    call void select (i1 icmp eq (i32* @global, i32* null), void ()* @f, void ()* @g)()
```
Neat that select can be consolidated into a call, though I wonder if it should be allowed given it could be canonicalized to be another IR instruction above it and maybe eliminate this edge case. 

Tangent aside, since in all these cases the call isn't part of the static callgraph `HasUnknownCall` needs to be set for correctness. Tests added in funcattrs-prop-unknown.ll (replacing funcattrs-prop-indirect.ll since we're handling more than just indirect here).


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:484
+        if (!CalleeSummary->fflags().NoUnwind ||
+            CallerSummary->fflags().MayThrow)
+          InferredFlags.NoUnwind = false;
----------------
tejohnson wrote:
> You've already set InferredFlags.NoUnwind to false above this loop in the case where MayThrow was set on the CallerSummary.
Good catch, this case should be querying CalleeSummary MayThrow.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:499
+          ++NumThinLinkNoRecurse;
+          CachedAttributes[V]->setNoRecurse();
+        }
----------------
tejohnson wrote:
> I think you can remove this and the below setNoUnwind() call on CachedAttributes[V] since presumably this points to one of the function summaries we update in the below loop.
Makes sense, removed. I like keeping the stats/debug tracking around though.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:537
+
+  for (Function &F : TheModule) {
+    const auto &GV = DefinedGlobals.find(F.getGUID());
----------------
tejohnson wrote:
> Consider consolidating this function with thinLTOResolvePrevailingInModule, to reduce the number of walks of the module and lookups into the DefinedGlobals map.
Good idea, merged and renamed `thinLTOResolvePrevailingInModule` to `thinLTOFinalizeInModule`


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop-indirect.ll:3
+; RUN: opt -thinlto-bc %s -thin-link-bitcode-file=%t1.thinlink.bc -o %t1.bc
+; RUN: llvm-lto2 run -disable-thinlto-funcattrs=0 %t1.bc -o %t.o -r %t1.bc,indirect,px -save-temps
+; RUN: llvm-dis -o - %t.o.1.3.import.bc | FileCheck %s
----------------
tejohnson wrote:
> Have a second version that tests with -thinlto-funcattrs-optimistic-indirect? I don't see a test for that option anywhere. Or maybe just remove that option - is it really needed?
Good point, option removed.


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop.ll:8
+;; 1. If external, linkonce_odr or weak_odr. We capture the attributes of the prevailing symbol for propagation.
+;; 2. If linkonce or weak, individual callers may optimize using different copies. However, the prevailing symbol will be what's in the final binary and thus we can propagate based off of that
+; RUN: llvm-lto2 run -disable-thinlto-funcattrs=0 %t/a.bc %t/b.bc %t/c.bc -o %t1 -save-temps \
----------------
tejohnson wrote:
> Since linkonce and weak are interposable, it isn't really correct to say that individual callers may optimize using different copies (we try to prevent this in the compiler since the are interposable).
True, this comment is a left-over from the first interpretation of linkage models which have since been fixed with your help :). I'll update this to reflect that only prevailing matters here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36850