-
Notifications
You must be signed in to change notification settings - Fork 40
Add IR printing functionality to the PassManager #494
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
Conversation
Thanks! I'll get this reviewed ASAP |
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
2ea0339
to
8c51c14
Compare
hir/src/pass/instrumentation.rs
Outdated
&mut self, | ||
pass: &dyn OperationPass, | ||
op: &OperationRef, | ||
changed: PostPassStatus, |
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.
I wonder if it might not be better to pass a reference to the PassExecutionState
here, and store the PostPassStatus
in the state instead. That will allow us to extend the data available to after-pass instrumentations in the future in a backwards-compatible way (i.e. no changes required here or any callers of this method).
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.
Great point! I incorporated this change in this commit: 54d3965
As a note, I avoided passing &mut PassExecutionState
to fn transform_spills
in order to minimize the places where PassExecutionState
can be mutated.
hir/src/pass/pass.rs
Outdated
@@ -136,6 +143,51 @@ where | |||
} | |||
} | |||
|
|||
#[derive(Debug, PartialEq, Clone, Copy)] | |||
pub enum PassIdentifier { |
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.
I don't think we need PassIdentifier
at all. We should be able to use the existing pass names as an identifier, and if we want faster comparisons, we can obtain a Symbol
from the raw string instead (i.e. crate::interner::Symbol
, not the trait), but I don't think the overhead of the string comparisons is enough to matter for this anyway (or if it is, we can deal with that when we start identifying hotspots in the compiler).
Looking at how this type is used, it isn't clear to me that there is any other motivating use aside from cheaper comparisons, but maybe as this PR evolved the original motivation went away while this type remained. What do you think about removing it?
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.
Great point, I removed the enum all together in this commit: 0646833.
The original intent behind the enum was to add type checking when comparing passes; but in retrospect, using the name()
method ended up being simpler.
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.
Got it, makes sense! I think for me the calculus boils down to two things:
- The set of available passes is not necessarily known to us, i.e. another compiler using our IR to lower to Miden might have their own set of passes, and we'd want any pass-related infrastructure to be used with those passes as well.
- In general, we want to avoid referencing stuff that exists in downstream crates from upstream crates in the dependency graph (e.g. in the enum as it was defined in
midenc_hir
, we were referencing passes that are defined in downstream crates, e.g.midenc_codegen_masm
).
So if you find yourself wanting to define a trait/enum/whatever and it violates one of those guidelines, we should probably discuss it, since we either need to do additional refactoring, or there might be a more suitable alternative. Doesn't mean we can't violate one of those rules, but we probably should have a really good reason to do so.
hir/src/pass/pass.rs
Outdated
|
||
#[derive(Debug, PartialEq, Clone, Copy)] | ||
pub enum PostPassStatus { | ||
IRUnchanged, |
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.
I'd simplify this to Unchanged
and Changed
, since there are no other variants. If there is ambiguity with future changes, we can deal with that then, otherwise it reads a bit unnecessarily verbose IMO.
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.
I completely agree; updated the enum variants here: 828a097
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 further testing, the enum did provide a very straight-forward way to validate user input, mainly in this function.
The enum + match
allowed for a "compile time" way to check if the user passed an invalid pass. Now after being removed, invalid passes passed to -Z print-ir-after-pass
are simply ignored.
For example:
midenc compile file.wasm -Z print-ir-after-pass=invalid
In order to validate user input, I pushed this commit: 247b1f7
Now, an error is raised:
midenc compile file.wasm -Z print-ir-after-pass=invalid
Error: x 'invalid' unrecognized pass.
However, I am hesistant about this implementation; since if any passes are added in the future, they would have to be added manually to that array.
What do you think? Does this situation merit the use of the enum? Or is this implementation sufficient?
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.
The set of possible passes is open, and isn't necessarily static even, so I think it is perfectly acceptable to simply ignore an invalid pass name.
However, if we did want to provide some feedback for an unintentional typo or something, it would likely need to be done by visiting all the nodes of the pass pipeline registered with the PassManager
to see if a pass with a given name is registered or not; and if not, raise a warning diagnostic. I'm not sure if the necessary APIs exist to do such a check right now, but I believe we could add them pretty easily. That doesn't need to happen as part of this PR though IMO.
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.
The set of possible passes is open, and isn't necessarily static even, so I think it is perfectly acceptable to simply ignore an invalid pass name.
However, if we did want to provide some feedback for an unintentional typo or something, it would likely need to be done by visiting all the nodes of the pass pipeline registered with the
PassManager
to see if a pass with a given name is registered or not; and if not, raise a warning diagnostic. I'm not sure if the necessary APIs exist to do such a check right now, but I believe we could add them pretty easily. That doesn't need to happen as part of this PR though IMO.
Got it, I did not take that into consideration. Thank you very much for the thorough explanation.
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.
Looks great! I have a few nits/questions/tweaks I've left comments for, but overall this is definitely a useful addition for debugging passes in the compiler!
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Now to compare pass equality, the `pass.name()` method is used. Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
… never constructred Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
469fcc2
to
e3365c2
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
hir/src/pass/manager.rs
Outdated
type Error = Report; | ||
|
||
fn try_from(options: &Options) -> Result<Self, Self::Error> { | ||
let available_passes = [ |
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.
I would remove this bit for now, as the set of passes is open - what dictates whether the pass name is "invalid" is whether or not such a pass has been registered with a specific PassManager
instance (and more precisely, somewhere in the pipeline being managed by that manager).
So if we want to have such a check, it will need to occur when the PassManager
is instantiated (or rather, when enable_ir_printing
is called). Since the IR printing is done via the trace infrastructure, we can simply emit a warning diagnostic when a pass name is not registered, rather than raising an error.
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.
Noted, I reverted this commit here: 71cad3a
I also updated the PR description with the latest changes.
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.
Looks good! I'm marking this approved, but will wait to merge until the invalid pass name validation is removed/modified based on my last comment. Nice work!
…fter-pass`." This reverts commit 247b1f7.
Usage
This PR implements the IR printing functionality, which can enabled via the use of flags passed to the compiler. The flags it introduces are:
-Z print-ir-after-all
,-Z print-ir-after-pass
and-Z print-ir-after-modified
.print-ir-after-all
: This enables IR printing for every pass.print-ir-after-pass
: This enables IR printing for a selected number passes.-Z print-ir-after-pass=canonicalizer,lift-control-flow
print-if-after-modified
: This will only allow a pass to print the IR if it modifies the IR it received as input.Here's an example output of the usage of these flags, these CLI examples are using the
fib.rs
available in the Miden book. Do note that the output here is excluding all the non-Print output, in reality the compiler is way more verbose.Print after every pass
print-ir-after-all
will enable IR printing after every compiler pass.Click here to see the CLI command
In order to reduce noise, the
-Z print-ir-after-modified
can be used:Click here to see the CLI command
Now, only passes that modify the IR will actually print. In the case of
fib.rs
those pases are:canonicalizer
,lift-control-flow
andsink-operand-defs
.Print after a subset of passes
And let's say that one is only interested in a subset of passes. For this, the
print-ir-after-pass
is added.The following will only print the IR after passes:
control-flow-sink
,canonicalizer
andlift-control-flow-pass
.Click here to see the CLI command
Furthermore, this last flag can also be combined with the
print-ir-after-modified
flag. This combination will only print IR if one of the selected passes modifies the IR.Click here to see the CLI command
Like we saw in the second example,
control-flow-sink
didn't modify the IR. So even though it is enabled, since-Z print-ir-after-modifed
was passed, it does not print IR.Implementation
For this PR, I used the already existing
Print
struct. Originally, it implemented thePass
trait, which enabledPrint
to be added after eachPass
, just as if it were any other compiler pass. Whilst this approach was straightforward to use for debugging a specific pass, it was not usable via the CLI and also required modifying the code manually.So,
Print
got its functionality moved fromPass
toPassInstrumentation
.PassInstrumentation
was an already present trait that allowed arbitray functions to be run before and/or after any pass. However, it was not implemented by any struct yet (If I'm not mistaken). ThePass
trait was actually removed fromPrint
, since if left implemented, it could lead to cases where IR printing was triggered twice: Once forPassInstrumentation
and another time forPass
.With this trait now in place,
Print
's methods could be invoked arbitrarily after every passed. The one thing that thePassInstrumentation
trait was missing was anOperationRef
, which actually holds theDisplay
trait implementation for each operation.Another thing that the issue required was the option to only print the IR after passes which modified its structure. That logic came in the form of the PostPassStatus enum, which was added to
PassExecutionState
. Now, everyPass::run_on_operation
updates said value via thePassExecutionState::set_post_pass_status
function to indicate whether the Pass modified the IR or not.To actually enable IR printing, one has to pass one of the flags described in the [#Usage] section. These will get stored in the
UnstableOptions
struct and will later get mapped to aPrint
struct in the new() method. If no Print struct is created, no print is enabled.