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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks - I know you are still working on this, but I had a few comments so far. I haven't had a chance to test it yet. Unfortunately, the nounwind propagation shouldn't do much on our side as we disable exceptions internally.



================
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
----------------
Do we expect the new noUnwind flag to show up here somewhere?


================
Comment at: clang/test/CodeGen/thinlto-distributed-cfi.ll:27
 ; CHECK-DIS: ^0 = module: (path: "{{.*}}thinlto-distributed-cfi.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: 7, typeIdInfo: (typeTests: (^2)))))
+; CHECK-DIS: ^1 = gv: (guid: 8346051122425466633, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 0, canAutoHide: 0), insts: 7, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0), typeIdInfo: (typeTests: (^2)))))
 ; CHECK-DIS: ^2 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: single, sizeM1BitWidth: 0))) ; guid = 7004155349499253778
----------------
Ditto


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:592
+      OS << "funcFlags: (";
+      OS << "ReadNone: " << this->ReadNone;
+      OS << ", ReadOnly: " << this->ReadOnly;
----------------
The capitalization here is off - the LLVM asm parsing expects lower camel case.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:354
+  // At this point, prevailing symbols have been resolved.
+  // Abort conditions where we go conservative.
+  // 1. Multiple instances with local linkage. Normally local linkage would be
----------------
Of the below cases, 1 can happen and we should just do something conservative. 2 and 3 should not and we can assert.


================
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
+
----------------
When does this happen?


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:405
+
+    if (FS) {
+      const auto &Linkage = GVS->linkage();
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:408
+      if (GlobalValue::isLocalLinkage(Linkage)) {
+        if (Local) {
+          errs() << "ThinLTO FunctionAttrs: Multiple Local Linkage, bailing on "
----------------
This can happen for the reasons mentioned in your comment above. No need for an error message, just early return {} for conservative behavior.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:421
+        assert(isPrevailing(VI.getGUID(), GVS.get()));
+        if (External) {
+          errs() << "ThinLTO FunctionAttrs: Multiple External Linkage, bailing "
----------------
This should never happen. The linker should already have given a multiply defined symbol error.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:443
+          // we'll keep the last one seen.
+          ODR = FS;
+        } else if (!Prevailing && !ODR) {
----------------
Won't we still have a copy marked prevailing? Wondering if the weak linkage cases can all be merged.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:445
+        } else if (!Prevailing && !ODR) {
+          hasNonODR = true;
+          nonODRCombinedFlags |= FS->fflags();
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:453
+
+  if (Local) {
+    assert(!External && !ODR);
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:568
+    if (FunctionSummary *FS = dyn_cast<FunctionSummary>(GV->second)) {
+      if (FS->fflags().ReadNone)
+        if (!F.doesNotAccessMemory())
----------------
ReadNone and ReadOnly aren't getting propagated yet afaict, so probably add a note to that effect here.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:576
+
+      if (FS->fflags().NoRecurse)
+        if (!F.doesNotRecurse())
----------------
Nit: suggest simplifying all of these to something like:
if (FS->fflags().NoRecurse && !F.doesNotRecurse())
  F.setDoesNotRecurse();


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

https://reviews.llvm.org/D36850