In addition to the module pass, this patch introduces a CG-SCC pass that runs the Attributor on a strongly connected component of the call graph (both old and new PM). The Attributor was always design to be used on a subset of functions which makes this patch mostly mechanical. The one change is that we give up `norecurse` deduction in the module pass in favor of doing it during the CG-SCC pass. This makes an interfaces simpler but can be revisited.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 41540 Build 41768: arc lint + arc unit
Event Timeline
Build result: FAILURE - Could not check out parent git hash "548fb3a7879311b94ef69aed9747cb8b6ef92374". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...
ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt
Build result: FAILURE - Could not check out parent git hash "7af3744fd9261d018af1ab25e2147d81505328ca". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...
ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt
I think changes for replaceAllUsesWith are not related to CG-SCC pass. For that part, LGTM. Could you split the patch?
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp | ||
---|---|---|
512 | Do you have any reason why you move Attributor here? |
Will do.
Will do.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp | ||
---|---|---|
512 | The module pass should run early to improve the passes before the inliner loop, e.g., createCalledValuePropagationPass below. |
Unit tests: fail. 60890 tests passed, 8 failed and 726 were skipped.
failed: LLVM.Other/new-pm-defaults.ll failed: LLVM.Other/new-pm-thinlto-defaults.ll failed: LLVM.Other/opt-O2-pipeline.ll failed: LLVM.Other/opt-O3-pipeline.ll failed: LLVM.Other/opt-Os-pipeline.ll failed: LLVM.Other/pass-pipelines.ll failed: LLVM.Transforms/Attributor/IPConstantProp/pthreads.ll failed: LLVM.Transforms/Attributor/IPConstantProp/return-constant.ll
clang-format: pass.
Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json
Unit tests: pass. 60898 tests passed, 0 failed and 726 were skipped.
clang-format: pass.
Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json
llvm/include/llvm/LinkAllPasses.h | ||
---|---|---|
195 | Space |
Unit tests: fail. 61272 tests passed, 6 failed and 736 were skipped.
failed: LLVM.Other/new-pm-defaults.ll failed: LLVM.Other/new-pm-thinlto-defaults.ll failed: LLVM.Other/opt-O2-pipeline.ll failed: LLVM.Other/opt-O3-pipeline.ll failed: LLVM.Other/opt-Os-pipeline.ll failed: LLVM.Other/pass-pipelines.ll
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Unit tests: fail. 61301 tests passed, 11 failed and 736 were skipped.
failed: LLVM.Transforms/Attributor/ArgumentPromotion/crash.ll failed: LLVM.Transforms/Attributor/ArgumentPromotion/fp80.ll failed: LLVM.Transforms/Attributor/ArgumentPromotion/nonzero-address-spaces.ll failed: LLVM.Transforms/Attributor/IPConstantProp/PR26044.ll failed: LLVM.Transforms/Attributor/IPConstantProp/fp-bc-icmp-const-fold.ll failed: LLVM.Transforms/Attributor/IPConstantProp/recursion.ll failed: LLVM.Transforms/Attributor/IPConstantProp/solve-after-each-resolving-undefs-for-function.ll failed: LLVM.Transforms/Attributor/liveness.ll failed: LLVM.Transforms/Attributor/noreturn_async.ll failed: LLVM.Transforms/Attributor/noreturn_sync.ll failed: LLVM.Transforms/Attributor/undefined_behavior.ll
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Unit tests: fail. 62144 tests passed, 19 failed and 811 were skipped.
failed: LLVM-Unit.Analysis/_/AnalysisTests/CGSCCPassManagerTest.TestUpdateCGAndAnalysisManagerForPasses8 failed: LLVM.Other/new-pm-thinlto-postlink-pgo-defaults.ll failed: LLVM.Other/new-pm-thinlto-postlink-samplepgo-defaults.ll failed: LLVM.Other/new-pm-thinlto-prelink-pgo-defaults.ll failed: LLVM.Other/new-pm-thinlto-prelink-samplepgo-defaults.ll failed: LLVM.Transforms/Attributor/ArgumentPromotion/dbg.ll failed: LLVM.Transforms/Attributor/ArgumentPromotion/live_called_from_dead.ll failed: LLVM.Transforms/Attributor/ArgumentPromotion/nonzero-address-spaces.ll failed: LLVM.Transforms/Attributor/ArgumentPromotion/pr27568.ll failed: LLVM.Transforms/Attributor/ArgumentPromotion/reserve-tbaa.ll failed: LLVM.Transforms/Attributor/IPConstantProp/pthreads.ll failed: LLVM.Transforms/Attributor/liveness.ll failed: LLVM.Transforms/Attributor/noalias.ll failed: LLVM.Transforms/Attributor/range.ll failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 0 of them are added as review comments below (why?).
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
Unit tests: unknown.
clang-tidy: fail. clang-tidy found 1 errors and 2 warnings. 0 of them are added as review comments below (why?).
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
Unit tests: fail. 62402 tests passed, 4 failed and 839 were skipped.
failed: LLVM.Other/new-pm-thinlto-postlink-pgo-defaults.ll failed: LLVM.Other/new-pm-thinlto-postlink-samplepgo-defaults.ll failed: LLVM.Other/new-pm-thinlto-prelink-pgo-defaults.ll failed: LLVM.Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 0 of them are added as review comments below (why?).
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp | ||
---|---|---|
512 | Have you considered running it directly after IPSCCP? Then it would benefit from the existing constant propagation. Also, running it unconditionally and then disabling it in the pass seems a bit odd (I think it was also discussed in another review which got abandoned IIRC). Is there a reason to not add it conditionally like we do with lots of other passes? |
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp | ||
---|---|---|
512 |
Its the classical conundrum. The nocapture and memory annotations can help IPSCCP and propagated constants can help the Attributor. I don't have numbers on both combinations so I picked one for now. My reasoning was as follows: The Attributor can propagate constants interprocedurally, so easy ones it will do on its own and it will also improve the result based on the propagation (the "conditional" part of SCCP). The hard ones are left for IPSCCP which has better changes given all the annotations. TBH, IPSCCP should run as part of the Attributor, not as a rewrite per se but with a thin layer to integrate it. Until then, I'm fine with any order as long as the Attributor Module pass runs early.
It was, I replied there as well [0]. I can move the flag that disables the Attributor into two files and replicate the conditional that checks it in total 5 times but I fail to see the benefit (both short and long term). In the short term it will almost not make a difference to the pass pipeline, sure the pass is not scheduled but as far as I can tell it does never invalidate anything so calling runXXX and immediately should be hard to find in a profile. In the hopefully short term the flag default will flip making it an awkward flag to have in the pass pipeline. If I remember correctly we have various passes that can be disabled via a flag and most of them have the flag in their respective pass file. This is no different except that we haven't flipped the default yet. Long story short, if there is a benefit or if you feel strongly about this I can move the conditional, otherwise I would just continue working on enabling it by default and once it is it's not awkward anymore. |
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp | ||
---|---|---|
512 |
Does SCCP use any of the memory annotations? I had a quick look and I could not find any place it looked at attributes. I might be missing something, but if it doesn't use any attributes, it should be fairly safe to move it?
Adding it conditionally in PassManagerBuilder has a few slight advantages IMO:
I think as long as it is disabled by default adding it conditionally provides better ergonomics. I think @fedor.sergeev had similar comments at D69596 |
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp | ||
---|---|---|
512 |
I was hoping it would look at noescape, readonly, and similar things, e.g., to resolve the loads from private globals that we talked about the other day. I haven't verified that. We can move it, given that we do not have data either way "it doesn't matter". I hoped the above would hold true and that would be a good argument but at the end of the day we can just collect statistics in both orders and compare.
That is already not true, as I noted before, a few examples: llvm/lib/Analysis/AliasAnalysis.cpp:61 llvm/lib/Transforms/Scalar/LoopDistribute.cpp:118 llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:67 llvm/lib/CodeGen/TypePromotion.cpp:50 Also, there will be at least two places (both PMs).
That is a fair point I guess.
Yes, though that doesn't seem to be much of a problem.
I don't think it is helping but I'll make a patch. |
Space