Page MenuHomePhabricator

[Attributor] Add an Attributor CG-SCC pass
ClosedPublic

Authored by jdoerfert on Nov 26 2019, 11:35 PM.

Details

Summary
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.

Diff Detail

Event Timeline

jdoerfert created this revision.Nov 26 2019, 11:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2019, 11:35 PM
jdoerfert updated this revision to Diff 231785.EditedDec 2 2019, 2:51 PM

Use the CallGraphUpdater (D70927) to leep the call graph up to date

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

jdoerfert retitled this revision from [Attributor] Add an Attributor CG-SCC pass NOTE: Not completely finished, see TODOs below! to [Attributor] Add an Attributor CG-SCC pass.Dec 9 2019, 12:08 AM
jdoerfert edited the summary of this revision. (Show Details)
jdoerfert updated this revision to Diff 232764.Dec 9 2019, 12:09 AM
jdoerfert edited the summary of this revision. (Show Details)

Small improvements + updated (=passing) tests

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

Can you add a testcase from https://reviews.llvm.org/D70737, please?

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
554

Do you have any reason why you move Attributor here?

jdoerfert marked an inline comment as done.Dec 11 2019, 11:58 AM

Can you add a testcase from https://reviews.llvm.org/D70737, please?

Will do.

I think changes for replaceAllUsesWith are not related to CG-SCC pass. For that part, LGTM. Could you split the patch?

Will do.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
554

The module pass should run early to improve the passes before the inliner loop, e.g., createCalledValuePropagationPass below.

jdoerfert updated this revision to Diff 233915.Dec 13 2019, 9:24 PM

Split out replaceAllUsesWith, added D70737 test case

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

Adjust the pass manager tests

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

hfinkel accepted this revision.Dec 18 2019, 5:16 AM
hfinkel added a subscriber: hfinkel.

LGTM

This revision is now accepted and ready to land.Dec 18 2019, 5:16 AM
xbolva00 added inline comments.Dec 18 2019, 6:29 AM
llvm/include/llvm/LinkAllPasses.h
197

Space

jdoerfert updated this revision to Diff 236475.Jan 6 2020, 3:27 PM

Rebase and better use of D72025

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

jdoerfert updated this revision to Diff 236601.Jan 7 2020, 8:38 AM
jdoerfert marked 2 inline comments as done.

Update tests and address comment

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.

jdoerfert updated this revision to Diff 241955.Feb 2 2020, 7:37 PM

One more rebase

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.

This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Feb 9 2020, 5:13 AM
fhahn added inline comments.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
554

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?

jdoerfert marked an inline comment as done.Feb 9 2020, 10:10 AM
jdoerfert added inline comments.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
554

Have you considered running it directly after IPSCCP? Then it would benefit from the existing constant propagation.

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.

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?

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.

[0] https://reviews.llvm.org/D69596#1842179

fhahn added inline comments.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
554

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.

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?

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.

Adding it conditionally in PassManagerBuilder has a few slight advantages IMO:

  • There's a single place to find the options to enable/disable various passes
  • Looking at the pass pipeline, you can be reasonably sure to know which passes are actually run. IMO having passes in the pipeline but not being run seems quite confusing (i.e. they will show up in the list of passes run, print-*-all will show them)
  • All attributor tests have to pass -disable-attributor=false, right?

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

jdoerfert marked an inline comment as done.Feb 10 2020, 7:52 AM
jdoerfert added inline comments.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
554

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?

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.

There's a single place to find the options to enable/disable various passes

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).

Looking at the pass pipeline, you can be reasonably sure to know which passes are actually run. IMO having passes in the pipeline but not being run seems quite confusing (i.e. they will show up in the list of passes run, print-*-all will show them)

That is a fair point I guess.

All attributor tests have to pass -disable-attributor=false, right?

Yes, though that doesn't seem to be much of a problem.

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

I don't think it is helping but I'll make a patch.