-
Notifications
You must be signed in to change notification settings - Fork 11.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CodeGen] Let PassBuilder
support machine passes
#76320
[CodeGen] Let PassBuilder
support machine passes
#76320
Conversation
b889cea
to
c0bfd9c
Compare
|
461fd04
to
5fdf4e8
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
5fdf4e8
to
5890047
Compare
} | ||
}; | ||
|
||
Result run(MachineFunction &IR, MachineFunctionAnalysisManager::Base &AM) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why MachineFunctionAnalysisManager
is designed this way. An InnerAnalysisManagerProxy<IRUnit, MachineFunction>
seems also OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yeah MachineFunctionAnalysisManager
does not fit in with the other analysis managers
it should be an alias for AnalysisManager<MachineFunction>
, and then we should have a proxy for module and function analyses (correct me if I'm misunderstanding your question)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the code, I seem to understand the reason for this, MachineFunctionPassManager
need get result from MachineModuleAnalysis
which is a module analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any specific reason it can't follow the rest of the new pass manager design and use proxies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MachineFunctionPassManager
can run on module directly (see the runin MachinePassManager
), which seems a contrary to the pass manager's design.
e29a493
to
e64daf6
Compare
22e44a8
to
c8c0931
Compare
at a high level, I think it makes sense to consolidate handling machine passes into PassBuilder however, it's unfortunate that this cements the Passes -> CodeGen dependency. it would be great if people could use PassBuilder to only run IR passes without pulling in all of CodeGen. but if |
Unfortunately, some IR pass and IR pass option tests need to verify the result in machine code form, an example is |
c8c0931
to
0a6fe74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg with nits
llvm/lib/Passes/PassBuilder.cpp
Outdated
@@ -73,6 +73,7 @@ | |||
#include "llvm/Analysis/TypeBasedAliasAnalysis.h" | |||
#include "llvm/Analysis/UniformityAnalysis.h" | |||
#include "llvm/CodeGen/CallBrPrepare.h" | |||
#include "llvm/CodeGen/CodeGenPassBuilder.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this include necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All dummy passes (DUMMY_*_PASS in MachinePassRegistry.def) are declared in this header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh man I don't think having those DUMMY_*_PASS
was a great idea. we should just add passes to the pipeline as we port them. we really shouldn't have to include CodeGenPassBuilder.h
from PassBuilder
can you just not #define DUMMY_*_PASS
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will remove them later.
@@ -463,8 +463,7 @@ class LLVMTargetMachine : public TargetMachine { | |||
} | |||
|
|||
virtual std::pair<StringRef, bool> getPassNameFromLegacyName(StringRef) { | |||
llvm_unreachable( | |||
"getPassNameFromLegacyName parseMIRPipeline is not overridden"); | |||
llvm_unreachable("getPassNameFromLegacyName is not overridden"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please submit this change separately (no review required)
@@ -0,0 +1,499 @@ | |||
//===- unittests/MIR/PassBuilderCallbacksTest.cpp - PB Callback Tests --===// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultra nit: line this up
c465a51
to
02c92b6
Compare
Address the comments above. |
`PassBuilder` would be a better place to parse MIR pipeline. We can reuse the code to support parsing pass with parameters and targets can reuse `registerPassBuilderCallbacks` to register the target specific passes. `PassBuilder` also has ability to check whether a Pass is a machine pass, so it can replace part of the work of `LLVMTargetMachine::getPassNameFromLegacyName`.
02c92b6
to
f8bd622
Compare
Remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg as is, the analysis manager stuff can be fixed later on as the current design is already in tree
02e17c2
to
c74a50f
Compare
Wrap TM with std::unique_ptr to avoid sanitizer error. |
`PassBuilder` would be a better place to parse MIR pipeline. We can reuse the code to support parsing pass with parameters and targets can reuse `registerPassBuilderCallbacks` to register the target specific passes. `PassBuilder` also has ability to check whether a Pass is a machine pass.
PassBuilder
would be a better place to parse MIR pipeline. We can reuse the code to support parsing pass with parameters and targets can reuseregisterPassBuilderCallbacks
to register the target specific passes.PassBuilder
also has ability to check whether a Pass is a machine pass, so it can replace part of the work ofLLVMTargetMachine::getPassNameFromLegacyName
.