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

[Di Mo via Phabricator via llvm-commits]

modimo marked 13 inline comments as done.
modimo added inline comments.


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll:39
 ; CHECK-DIS: ^0 = module: (path: "{{.*}}thinlto-distributed-cfi-devirt.ll.tmp.o", hash: ({{.*}}, {{.*}}, {{.*}}, {{.*}}, {{.*}}))
-; CHECK-DIS: ^1 = gv: (guid: 8346051122425466633, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 0, canAutoHide: 0), insts: 18, typeIdInfo: (typeTests: (^2), typeCheckedLoadVCalls: (vFuncId: (^2, offset: 8), vFuncId: (^2, offset: 0))))))
+; CHECK-DIS: ^1 = gv: (guid: 8346051122425466633, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 0, canAutoHide: 0), insts: 18, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0), typeIdInfo: (typeTests: (^2), typeCheckedLoadVCalls: (vFuncId: (^2, offset: 8), vFuncId: (^2, offset: 0)))))) 
 ; CHECK-DIS: ^2 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes, sizeM1BitWidth: 7), wpdResolutions: ((offset: 0, wpdRes: (kind: branchFunnel)), (offset: 8, wpdRes: (kind: singleImpl, singleImplName: "_ZN1A1nEi"))))) ; guid = 7004155349499253778
----------------
tejohnson wrote:
> Do we expect the new noUnwind flag to show up here somewhere?
Yeah. I changed the printing logic back to not print anything when no flags are set since quite a few tests depended on this behavior so these tests ultimately remain unchanged. I added a new test here to explicitly check that the flags from propagation show up correctly.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:592
+      OS << "funcFlags: (";
+      OS << "ReadNone: " << this->ReadNone;
+      OS << ", ReadOnly: " << this->ReadOnly;
----------------
tejohnson wrote:
> The capitalization here is off - the LLVM asm parsing expects lower camel case.
Changing back to camel case, also added parsing code for noUnwind.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:373
+  //   b. If there's an ODR copy, pick it
+  //   c. Otherwise, merge all the weak copies together
+
----------------
tejohnson wrote:
> When does this happen?
Good question. I looked further into exactly what triggers this in clang self-build and there are summaries which are AvailableExternally but have no Prevailing copy. I added more about this in the source comments but TL;DR these end up being edge cases that can be ignored. 

Changed the logic to go conservative in these cases.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:405
+
+    if (FS) {
+      const auto &Linkage = GVS->linkage();
----------------
tejohnson wrote:
> If for some reason (GUID alias due to local name without paths or other rare case) we get a non FunctionSummary, just early return {} and remove a level of nesting?
Sounds good, removed


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:443
+          // we'll keep the last one seen.
+          ODR = FS;
+        } else if (!Prevailing && !ODR) {
----------------
tejohnson wrote:
> Won't we still have a copy marked prevailing? Wondering if the weak linkage cases can all be merged.
Yeah, there will still be a copy that's prevailing. Reading through the linkage descriptions again and also those in `FunctionImportGlobalProcessing::getLinkage`:

1. I think with External/WeakODR/LinkOnceODR once the prevailing is found use that copy

2. For Weak/LinkOnce even with a prevailing copy I don't know if the copy ultimately used will be prevailing. I'm wondering if a native definition could be the victor in which case we just can't propagate off these functions. 

WDYT about (2)? For C++ at least these don't seem to really exist and testing with Clang self-build I'm not seeing this kick in anywhere. I added a flag to specifically disable this case so it's easy to test out the differences.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:445
+        } else if (!Prevailing && !ODR) {
+          hasNonODR = true;
+          nonODRCombinedFlags |= FS->fflags();
----------------
tejohnson wrote:
> I don't understand this case. If there is no prevailing symbol in the IR for this GUID then presumably it should not have been marked live. Are you seeing this kick in?
Same answer as above:

Good question. I looked further into exactly what triggers this in clang self-build and there are summaries which are AvailableExternally but have no Prevailing copy. I added more about this in the comments but TL;DR these end up being edge cases that can be ignored.

Changed the logic to go conservative in these cases.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:453
+
+  if (Local) {
+    assert(!External && !ODR);
----------------
tejohnson wrote:
> In all of the cases here, other than the hasNonODR case which I don't understand (yet), we should have a single prevailing FunctionSummary. Can we just cache that, rather than e.g. copying all of its callees?
Good idea, I'll do that. The combined case I suspect will be quite rare in C++ code so the cache only captures the FunctionSummaries and leaves generating a merged callee graph at use time.


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

https://reviews.llvm.org/D36850