This is an archive of the discontinued LLVM Phabricator instance.

[NPM] Add target specific hook to add passes for New Pass Manager
ClosedPublic

Authored by quic_aankit on Sep 23 2020, 12:13 AM.

Details

Summary

The patch adds a new TargetMachine member "registerPassBuilderCallbacks" for targets to add passes to the pass pipeline using the New Pass Manager (similar to adjustPassManager for the Legacy Pass Manager).

Diff Detail

Event Timeline

quic_aankit created this revision.Sep 23 2020, 12:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2020, 12:13 AM
quic_aankit requested review of this revision.Sep 23 2020, 12:13 AM

Makes sense, but do you have an example usage of this?

Makes sense, but do you have an example usage of this?

I wanted to add the equivalent of adjustPassManager for Hexagon. Should I make that change in this same patch itself?

Makes sense, but do you have an example usage of this?

I wanted to add the equivalent of adjustPassManager for Hexagon. Should I make that change in this same patch itself?

Yes, I think that makes sense to get a better picture of how it would be used.

quic_aankit updated this revision to Diff 294227.EditedSep 24 2020, 10:18 PM

Added HexagonVectorLoopCarriedReusePass to registerPassBuilderCallbacks

I think I'm still missing how exactly this will fit into the pipeline. As in where is registerPassBuilderCallbacks() going to be called?

And a test like llvm/test/CodeGen/AMDGPU/opt-pipeline.ll for the new pass manager route would be nice to make sure this actually works.
something like RUN: opt -passes='default<O0>' -mtriple=... -disable-output -disable-verify -debug-pass-manager. Maybe checking all the normal passes isn't necessary like in the existing opt-pipeline.ll isn't necessary, just need to check that the custom passes (e.g. HexagonVectorLoopCarriedReusePass) are actually added.

llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp
278 ↗(On Diff #294227)

this is typically fully capitalized

I think I'm still missing how exactly this will fit into the pipeline. As in where is registerPassBuilderCallbacks() going to be called?

And a test like llvm/test/CodeGen/AMDGPU/opt-pipeline.ll for the new pass manager route would be nice to make sure this actually works.
something like RUN: opt -passes='default<O0>' -mtriple=... -disable-output -disable-verify -debug-pass-manager. Maybe checking all the normal passes isn't necessary like in the existing opt-pipeline.ll isn't necessary, just need to check that the custom passes (e.g. HexagonVectorLoopCarriedReusePass) are actually added.

My understanding is that this API gives other applications chance to modify the pass pipeline. HexagonVectorLoopCarriedReuse pass in not included in the pass pipeline for any optimization level, but we used to add the pass using adjustPassManager in Halide. That is why I don't think I can use the suggested method to test if the pass is being added

quic_aankit marked an inline comment as done.

I think I'm still missing how exactly this will fit into the pipeline. As in where is registerPassBuilderCallbacks() going to be called?

And a test like llvm/test/CodeGen/AMDGPU/opt-pipeline.ll for the new pass manager route would be nice to make sure this actually works.
something like RUN: opt -passes='default<O0>' -mtriple=... -disable-output -disable-verify -debug-pass-manager. Maybe checking all the normal passes isn't necessary like in the existing opt-pipeline.ll isn't necessary, just need to check that the custom passes (e.g. HexagonVectorLoopCarriedReusePass) are actually added.

My understanding is that this API gives other applications chance to modify the pass pipeline. HexagonVectorLoopCarriedReuse pass in not included in the pass pipeline for any optimization level, but we used to add the pass using adjustPassManager in Halide. That is why I don't think I can use the suggested method to test if the pass is being added

HexagonTargetMachine::adjustPassManager() adds some Hexagon-specific passes to the optimization pipeline.

$ ./build/rel/bin/opt -disable-output -debug-pass=Structure -O1 /dev/null -mtriple=hexagon |& grep -i hexagon
Pass Arguments:  -targetlibinfo -tti -targetpassconfig -tbaa -scoped-noalias-aa -assumption-cache-tracker -profile-summary-info -forceattrs -inferattrs -ipsccp -called-value-propagation -globalopt -domtree -mem2reg -deadargelim -domtree -basic-aa -aa -loops -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -simplifycfg -basiccg -globals-aa -prune-eh -always-inline -function-attrs -domtree -sroa -basic-aa -aa -memoryssa -early-cse-memssa -simplifycfg 
-domtree -basic-aa -aa -loops -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -libcalls-shrinkwrap -loops -postdomtree -branch-prob -block-freq -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -pgo-memop-opt -simplifycfg -reassociate -domtree -loops -loop-simplify -lcssa-verification -lcssa -basic-aa -aa -scalar-evolution -loop-rotate -memoryssa -lazy-branch-prob -lazy-block-freq -licm -loop-unswitch -simplifycfg -domtree -basic-aa -aa -loops -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -loop-simplify -lcssa-verification -lcssa -scalar-evolution -indvars -loop-idiom -hexagon-loop-idiom -domtree -loops -loop-simplify -lcssa-verification -lcssa -scalar-evolution -loop-deletion -loop-unroll -hexagon-vlcr -phi-values -basic-aa -aa -memdep -memcpyopt -sccp -demanded-bits -bdce -aa -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -postdomtree -adce -simplifycfg -domtree -basic-aa -aa -loops -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -barrier -basiccg -rpo-function-attrs -globalopt -globaldce -basiccg -globals-aa -domtree -float2int -lower-constant-intrinsics -domtree -loops -loop-simplify -lcssa-verification -lcssa -basic-aa -aa -scalar-evolution -loop-rotate -loop-accesses -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -loop-distribute -postdomtree -branch-prob -block-freq -scalar-evolution -basic-aa -aa -loop-accesses -demanded-bits -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -inject-tli-mappings -loop-vectorize -loop-simplify -scalar-evolution -aa -loop-accesses -lazy-branch-prob -lazy-block-freq -loop-load-elim -basic-aa -aa -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -simplifycfg -domtree -vector-combine -basic-aa -aa -loops -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -loop-simplify -lcssa-verification -lcssa -scalar-evolution -loop-unroll -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instcombine -memoryssa -loop-simplify -lcssa-verification -lcssa -scalar-evolution -lazy-branch-prob -lazy-block-freq -licm -opt-remark-emitter -transform-warning -alignment-from-assumptions -strip-dead-prototypes -cg-profile -domtree -loops -postdomtree -branch-prob -block-freq -loop-simplify -lcssa-verification -lcssa -basic-aa -aa -scalar-evolution -block-freq -loop-sink -lazy-branch-prob -lazy-block-freq -opt-remark-emitter -instsimplify -div-rem-pairs -simplifycfg -verify
          Recognize Hexagon-specific loop idioms
          Hexagon-specific loop carried reuse for HVX vectors

opt.cpp contains TM->adjustPassManager(Builder); for the legacy PM path.
The thing you're adding here I thought was the equivalent for the new pass manager. Am I missing something?

My understanding is that this API gives other applications chance to modify the pass pipeline.

This is to allow target-specific passes to be inserted into the pass pipeline. This is the exact equivalent of adjustPassManager for the legacy manager. See EmitAssemblyHelper::CreatePasses in clang/lib/CodeGen/BackendUtil.cpp.

I think I'm still missing how exactly this will fit into the pipeline. As in where is registerPassBuilderCallbacks() going to be called?

Perhaps EmitAssemblyHelper::EmitAssemblyWithNewPassManager in BackendUtil.cpp. PassBuilder actually has TM as a member, so maybe from somewhere inside of PassBuilder itself, but I'm not sure whether that's the right thing to do.

I think I'm still missing how exactly this will fit into the pipeline. As in where is registerPassBuilderCallbacks() going to be called?

Perhaps EmitAssemblyHelper::EmitAssemblyWithNewPassManager in BackendUtil.cpp. PassBuilder actually has TM as a member, so maybe from somewhere inside of PassBuilder itself, but I'm not sure whether that's the right thing to do.

Anywhere adjustPassManager() is called should have a corresponding call to registerPassBuilderCallbacks() in the NPM path. So yes EmitAssemblyHelper::EmitAssemblyWithNewPassManager() in BackendUtil.cpp, but also since opt.cpp calls adjustPassManager(), the corresponding NPM path in NewPMDriver.cpp should also call registerPassBuilderCallbacks(). Both places have access to a TM.

This should be done either before or in this patch, or else we can't test it and make sure it works.

I think I'm still missing how exactly this will fit into the pipeline. As in where is registerPassBuilderCallbacks() going to be called?

Perhaps EmitAssemblyHelper::EmitAssemblyWithNewPassManager in BackendUtil.cpp. PassBuilder actually has TM as a member, so maybe from somewhere inside of PassBuilder itself, but I'm not sure whether that's the right thing to do.

Anywhere adjustPassManager() is called should have a corresponding call to registerPassBuilderCallbacks() in the NPM path. So yes EmitAssemblyHelper::EmitAssemblyWithNewPassManager() in BackendUtil.cpp, but also since opt.cpp calls adjustPassManager(), the corresponding NPM path in NewPMDriver.cpp should also call registerPassBuilderCallbacks(). Both places have access to a TM.

This should be done either before or in this patch, or else we can't test it and make sure it works.

Sorry I think I misunderstood the usage of adjustPassManager earlier. I'll add the calls in both opt and clang and add the test case as well. Thanks!

This looks good, just a couple nits

llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp
280 ↗(On Diff #295151)

We should probably thread the DebugPassManager bool into here as a new param to registerPassBuilderCallbacks().

llvm/test/CodeGen/Hexagon/registerpassbuildercallbacks.ll
3 ↗(On Diff #295151)

I think just the Running pass: ...Pass part is sufficient, the on Loop ... seems brittle

6 ↗(On Diff #295151)

How did you come up with this IR?

I've typically seen something like the IR in llvm/test/Other/new-pm-defaults.ll to test this sort of stuff.

quic_aankit marked 2 inline comments as done.
quic_aankit added inline comments.
llvm/test/CodeGen/Hexagon/registerpassbuildercallbacks.ll
6 ↗(On Diff #295151)

I generated the .ll through C source file.

Is it better to add the testcase to llvm/test/Other/new-pm-defaults.ll itself?

aeubanks added inline comments.Sep 30 2020, 7:19 AM
llvm/test/CodeGen/Hexagon/registerpassbuildercallbacks.ll
6 ↗(On Diff #295151)

I think copying that here would be better rather than using something generated by clang. That IR was specifically constructed for this sort of test.

aeubanks accepted this revision.Sep 30 2020, 11:16 AM

One nit, otherwise lgtm

Thanks for doing this!

llvm/include/llvm/Target/TargetMachine.h
300

I'd prefer no default value so callers don't forget to pass it since there should always be a user option to enable pass manager logging, which is already somewhat enforced by no default value for debug logging in the constructors for the various pass managers.

This revision is now accepted and ready to land.Sep 30 2020, 11:16 AM
quic_aankit marked an inline comment as done.

@aeubanks Thanks for the review. Can you also commit the patch on my behalf?

quic_aankit marked an inline comment as done.Sep 30 2020, 11:27 AM

Makes sense. Removed the default value

Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2020, 1:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@aeubanks Thanks for the review. Can you also commit the patch on my behalf?

Committed in ce5379f0f0675592fd10a522009fd5b1561ca72b.

Thank you, Ankit!

I think this, and similar recent commits, are causing the shared library builds to fail some tests if this code gets linked into libLLVM.so: https://bugs.llvm.org/show_bug.cgi?id=48181. I assume it might actually a bug in ld (GNU Binutils for Ubuntu 2.34), as I don't understand the linker behavior there?

I think this, and similar recent commits, are causing the shared library builds to fail some tests if this code gets linked into libLLVM.so: https://bugs.llvm.org/show_bug.cgi?id=48181. I assume it might actually a bug in ld (GNU Binutils for Ubuntu 2.34), as I don't understand the linker behavior there?

I'm trying to replicate the issue.