Page MenuHomePhabricator

[NewPM] Add C bindings for new pass manager
ClosedPublic

Authored by supergrecko on May 9 2021, 11:02 AM.

Details

Summary

This patch contains the bare minimum to run the new Pass Manager from the LLVM-C APIs. It does not feature PGOOptions, PassPlugins or Debugify in its current state. Bugzilla: PR48499

Diff Detail

Event Timeline

supergrecko created this revision.May 9 2021, 11:02 AM
supergrecko requested review of this revision.May 9 2021, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2021, 11:02 AM

Not quite sure how we want to register the StandardInstrumentations here... Right now it's not registering any PassInstrumentationCallbacks at all because I'm not too sure how we wanted to handle that either.

StandardInstrumentations::registerCallbacks takes the FunctionAnalysisManager which means we'd need to have the FunctionAnalysisManager instance at the time the StandardInstrumentation is registered. Not sure how to move forward here. Let me know what you think.

llvm/lib/Passes/PassBuilderBindings.cpp
64

I made the choice to dispose of the PassBuilder once you've run it over a module because we're setting up a whole bunch of things like AnalysisManagers (and thus changing the underlying PB) based on the Debug and VerifyEach parameters which could be different if the client was to call LLVMRunpassBuilder again. Let me know if you believe this is wrong.

myhsu added a subscriber: myhsu.May 9 2021, 2:08 PM

StandardInstrumentations::registerCallbacks takes the FunctionAnalysisManager which means we'd need to have the FunctionAnalysisManager instance at the time the StandardInstrumentation is registered. Not sure how to move forward here. Let me know what you think.

I think you can build PassBuilder with an uninitialized PassInstrumentationCallbacks object and retrieve it via PassBuilder::getPassInstrumentationCallbacks before populating it with StandardInstrumentation when FunctionAnalysisManager is available (getPassInstrumentationCallbacks is marked const but the returned pointer is not so I'm not complaining :-P. Plus, the comment for getPassInstrumentationCallbacks also hints this usage).

Also I'm wondering what about other methods in PassBuilder? For instance those "buildXXXPipeline" methods. Do you have any plan/roadmap for adding them? (I'm not saying you should add all of them in a single patch but just curious :-) Since I actually use those methods more often than parsePassPipeline)

llvm/lib/Passes/PassBuilderBindings.cpp
40

I feel this name is a little bit mis-leading since you're only calling parsePassPipeline here, which is not the only features provided by PassBuilder

I think you can build PassBuilder with an uninitialized PassInstrumentationCallbacks object and retrieve it via PassBuilder::getPassInstrumentationCallbacks before populating it with StandardInstrumentation when FunctionAnalysisManager is available (getPassInstrumentationCallbacks is marked const but the returned pointer is not so I'm not complaining :-P. Plus, the comment for getPassInstrumentationCallbacks also hints this usage).

That sounds like the solution I was looking for. Excellent, thank you!

Also I'm wondering what about other methods in PassBuilder? For instance those "buildXXXPipeline" methods. Do you have any plan/roadmap for adding them? (I'm not saying you should add all of them in a single patch but just curious :-) Since I actually use those methods more often than parsePassPipeline)

I believe the initial plan was to provide an interface to the parserPassPipeline bits of the PassBuilder so I haven't thought too much about it. That's definitely something we could do, in which case I think we could just return a ModulePassManager from the (currently named) LLVMRunPassBuilder function. Arthur and I discussed separating the PassBuilder and ModulePassManager APIs and this seems like a good reason to do that.

llvm/lib/Passes/PassBuilderBindings.cpp
40

Yeah, I'm sure there are better names than LLVMRunPassBuilder. I named it LLVMRunPassBuilder because it ends up running the PM, but a name like LLVMParsePassPipeline seems like a better fit. It would also make sense to name it something along those lines if we separate the ModulePassManager API out of this function.

myhsu added a comment.May 10 2021, 9:12 AM

Also I'm wondering what about other methods in PassBuilder? For instance those "buildXXXPipeline" methods. Do you have any plan/roadmap for adding them? (I'm not saying you should add all of them in a single patch but just curious :-) Since I actually use those methods more often than parsePassPipeline)

I believe the initial plan was to provide an interface to the parserPassPipeline bits of the PassBuilder so I haven't thought too much about it. That's definitely something we could do, in which case I think we could just return a ModulePassManager from the (currently named) LLVMRunPassBuilder function. Arthur and I discussed separating the PassBuilder and ModulePassManager APIs and this seems like a good reason to do that.

Yeah separating them definitely sounds better

I'm very grateful there are tests, some of the other C bindings I've worked with never had any :)

Also I'm wondering what about other methods in PassBuilder? For instance those "buildXXXPipeline" methods. Do you have any plan/roadmap for adding them? (I'm not saying you should add all of them in a single patch but just curious :-) Since I actually use those methods more often than parsePassPipeline)

I believe the initial plan was to provide an interface to the parserPassPipeline bits of the PassBuilder so I haven't thought too much about it. That's definitely something we could do, in which case I think we could just return a ModulePassManager from the (currently named) LLVMRunPassBuilder function. Arthur and I discussed separating the PassBuilder and ModulePassManager APIs and this seems like a good reason to do that.

Yeah separating them definitely sounds better

I still think keeping them together is a better idea.
For buildXXXPipeline methods, we can expose those via the pass parsing interface by adding them to PassRegistry.def.

The weird quirk of disposing the PassBuilder when it's passed to LLVMRunPassBuilder and the awkward naming of LLVMRunPassBuilder are easily resolved if we combine the two, since PassBuilder will be internal to that one function, and LLVMRunPasses seems nice

llvm/lib/Passes/PassBuilderBindings.cpp
70

these options can't be all set in one exposed interface, they each need separate functions (like PassManagerBuilder options)
if we add new options in the future, we can't just update a C function signature

The weird quirk of disposing the PassBuilder when it's passed to LLVMRunPassBuilder and the awkward naming of LLVMRunPassBuilder are easily resolved if we combine the two, since PassBuilder will be internal to that one function, and LLVMRunPasses seems nice

LLVMRunPasses would be nice and descriptive for its task. I'm not too sure I understood what you mean by "the quirk of disposing the PassBuilder (...)"; would it be preferable to have the client dispose it themselves? If the PassBuilder is re-usable after a run it would probably not make sense to get rid of it, but you know more about that than I do. Let me know what you think is the correct decision here.

llvm/lib/Passes/PassBuilderBindings.cpp
70

Good catch. I'll refactor that into LLVMPipelineTuningOptionsSetX functions.

The weird quirk of disposing the PassBuilder when it's passed to LLVMRunPassBuilder and the awkward naming of LLVMRunPassBuilder are easily resolved if we combine the two, since PassBuilder will be internal to that one function, and LLVMRunPasses seems nice

LLVMRunPasses would be nice and descriptive for its task. I'm not too sure I understood what you mean by "the quirk of disposing the PassBuilder (...)"; would it be preferable to have the client dispose it themselves? If the PassBuilder is re-usable after a run it would probably not make sense to get rid of it, but you know more about that than I do. Let me know what you think is the correct decision here.

It's weird that clients only sometimes manually dispose of the PassBuilder. For example, if a language with destructors were to wrap LLVMPassBuilderRef, there'd be weird logic in the destructor on when to call LLVMDisposePassBuilder. I'm not worried about supporting reusing a PassBuilder.
IMO we should only have the creation/setup of options (in this case PTO, maybe some other things like DebugLogging and TargetMachine), then pass those options and the pass pipeline string and the module to a LLVMRunPasses. This simplifies everything and we don't have to worry about any LLVMPassBuilderRefs since we won't be exposing it.

supergrecko added a comment.EditedMay 11 2021, 5:44 AM

The weird quirk of disposing the PassBuilder when it's passed to LLVMRunPassBuilder and the awkward naming of LLVMRunPassBuilder are easily resolved if we combine the two, since PassBuilder will be internal to that one function, and LLVMRunPasses seems nice

LLVMRunPasses would be nice and descriptive for its task. I'm not too sure I understood what you mean by "the quirk of disposing the PassBuilder (...)"; would it be preferable to have the client dispose it themselves? If the PassBuilder is re-usable after a run it would probably not make sense to get rid of it, but you know more about that than I do. Let me know what you think is the correct decision here.

It's weird that clients only sometimes manually dispose of the PassBuilder. For example, if a language with destructors were to wrap LLVMPassBuilderRef, there'd be weird logic in the destructor on when to call LLVMDisposePassBuilder. I'm not worried about supporting reusing a PassBuilder.
IMO we should only have the creation/setup of options (in this case PTO, maybe some other things like DebugLogging and TargetMachine), then pass those options and the pass pipeline string and the module to a LLVMRunPasses. This simplifies everything and we don't have to worry about any LLVMPassBuilderRefs since we won't be exposing it.

Aha, gotcha. I'll make it so the PassBuilder is kept private to LLVMRunPasses, removing LLVMPassBuilderRef and friends. That should result in an API looking like this:

LLVMErrorRef LLVMRunPasses(LLVMTargetMachineRef TM,
                           LLVMPipelineTuningOptionsRef PTO, LLVMModuleRef M,
                           LLVMBool Debug, LLVMBool VerifyEach,
                           const char *Passes);

Arcanist question: to update an existing revision on Phab, would I run arc diff --update D102136 with my new changes?

supergrecko updated this revision to Diff 345089.EditedMay 13 2021, 4:50 AM

[NewPM][C-bindings] Keep PassBuilder private to LLVMRunPasses and enable StandardInstrumentations

Edit: Looks like I only pushed my latest local git commit with arc diff by accident. This diff should be ignored.

supergrecko updated this revision to Diff 345091.EditedMay 13 2021, 5:02 AM

[NewPM][C-bindings] Keep PassBuilder private to LLVMRunPasses and enable StandardInstrumentation

Edit: I once again messed up my arc diff. This one should also be ignored.

[NewPM][C-bindings] Keep PassBuilder private to LLVMRunPasses and enable StandardInstrumentation

Okay so I had some troubles with arc, but I think I finally got it right. I've now refactored LLVMRunPassBuilder into LLVMRunPasses with the changes we went through earlier and I've added the StandardInstrumentations inside of that function.

Apologies for the arcanist trouble

supergrecko edited the summary of this revision. (Show Details)May 13 2021, 7:55 AM

looks pretty good, just one nit

llvm/include/llvm-c/Transforms/PassBuilder.h
46

We should also move Debug and VerifyEach into their own separate options. We might make changes to these options (as I'm doing right now for DebugLogging), so we shouldn't tie down the exact option types into the main LLVMRunPasses API.

Maybe instead of LLVMPipelineTuningOptionsRef, we just have some generic LLVMPassBuilderOptionsRef which includes a PTO and these Debug/VerifyEach bools. That way if we want to change exactly how Debug works in the future, we can change the LLVMPassBuilderOptions underlying struct, and we only need to deprecate a separate API, not the main LLVMRunPasses. (or whatever naming you'd like)

[NewPM] Extract LLVMRunPasses options into LLVMPassBuilderOptionsRef

looks pretty good, just one nit

Good point. I have updated LLVMRunPasses to accept an LLVMPassBuilderOptionsRef which is currently a triplet of VerifyEach, DebugLogging and PipelineTuningOptions. I've made the construction/destruction/modification of this struct on the C-side equal to PipelineTuningOptions.

The llvm::LLVMPassBuilderOptions class is kept private to the bindings source because there shouldn't be a reason for anybody else to use this class. Right now I am using an assert for the LLVMPassBuilderOptionsRef->PTO to be present, but we could also change this to an LLVMErrorRef if that's preferable.

aeubanks added inline comments.May 14 2021, 10:52 AM
llvm/lib/Passes/PassBuilderBindings.cpp
36

can we just make this PipelineTuningOptions PTO? no need for a separate PTO instance. it makes the API more complicated than necessary (for example if someone just wants the default settings)
and we can just pass LLVMPassBuilderOptionsRef to the PTO APIs instead of exposing LLVMPipelineTuningOptionsRef

[NewPM] Abstract PipelineTuningOptions away into LLVMPassBuilderOptions

aeubanks accepted this revision.May 14 2021, 11:56 AM

looks good aside from one super duper nit
do you have commit permission?

llvm/include/llvm-c/Transforms/PassBuilder.h
49–51

super duper nit, I'd move the module first, passes string second, TM third, and options fourth, so that the more important things are the earlier parameters

This revision is now accepted and ready to land.May 14 2021, 11:56 AM

[NewPM] Re-arrange order of arguments for LLVMRunPasses

looks good aside from one super duper nit
do you have commit permission?

No, I do not have commit access. Not sure what information you need to attribute me as commit author, email is me@supergrecko.com, GitHub username is supergrecko, name is Mats Larsen. Let me know if there's anything else you need.

arc patch seems to have preserved your email/name so that's all taken care of.
Thanks for doing this!
I'll wait a bit to see if anybody else has comments.

Found a tiny mistake in one of the comments, will fix.

llvm/include/llvm-c/Transforms/PassBuilder.h
43–47

Just realized that these lines don't make any sense anymore after we moved them out into PassBuilderOptions.

[NewPM] Remove documentation comments for parameters that were refactored into LLVMPassBuilderOptionsRef

This revision was landed with ongoing or failed builds.May 17 2021, 10:49 AM
This revision was automatically updated to reflect the committed changes.

[NewPM] Remove documentation comments for parameters that were refactored into LLVMPassBuilderOptionsRef

I think it doesn't build. Could you check if this is the case? @aeubanks

This doesn't build. I reverted it for now in 0b3397787222. Ideally, build locally, failing that wait for the presubmit bot to complete, failing that please watch bots after committing.

sorry about that, I'll be more diligent about actually making sure a patch works before submitting

It looks like my local branch was a bit behind tip of tree when I started working on it which meant that https://reviews.llvm.org/rG34a8a437bf20e0a340c19ed1fdb9cca584d43da1 wasn't checked into my local branch.

My bad for not rebasing against main while working on it and thank you @aeubanks for fixing it!

hctim added a subscriber: hctim.May 17 2021, 1:35 PM

Looks like this causes an ASan buildbot failure. I've left inline comments:

https://lab.llvm.org/buildbot/#/builders/5/builds/7712/

=================================================================
==88767==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 25 byte(s) in 1 object(s) allocated from:
    #0 0x212ffc4 in strdup /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors.cpp:439:3
    #1 0x2b8c0cf in LLVMGetDefaultTargetTriple /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/TargetMachineC.cpp:246:10
    #2 0x21725ed in PassBuilderCTest::SetUp() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/unittests/Passes/PassBuilderBindingsTest.cpp:19:26
    #3 0x34f7e88 in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #4 0x34f7e88 in testing::Test::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2503:3
    #5 0x34fa90c in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2684:11
    #6 0x34fbcd0 in testing::TestSuite::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2816:28
    #7 0x352b1b1 in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:5338:44
    #8 0x352961c in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #9 0x352961c in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
    #10 0x34e58e0 in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2473:46
    #11 0x34e58e0 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
    #12 0x7f76b899d09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
SUMMARY: AddressSanitizer: 25 byte(s) leaked in 1 allocation(s).
llvm/unittests/Passes/PassBuilderBindingsTest.cpp
19
/*===-- Triple ------------------------------------------------------------===*/
/** Get a triple for the host machine as a string. The result needs to be
  disposed with LLVMDisposeMessage. */
char* LLVMGetDefaultTargetTriple(void);

Memory leak here caused by not calling LLVMDisposeMessage(Triple).

aeubanks added inline comments.May 17 2021, 1:58 PM
llvm/unittests/Passes/PassBuilderBindingsTest.cpp
19

FWIW, it looks like this has broken the fast Windows builder: https://lab.llvm.org/buildbot/#/builders/86/builds/13664

FWIW, it looks like this has broken the fast Windows builder: https://lab.llvm.org/buildbot/#/builders/86/builds/13664

5c291482ec8bcd686044ebc0d4cffe7bf769521c should hopefully fix that