[PATCH] D36850: [ThinLTO] Add norecurse function attribute propagation
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 11 16:57:34 PDT 2021
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
More information about the llvm-commits
mailing list