-
Notifications
You must be signed in to change notification settings - Fork 0
Adding IR printing functionality to the PassManager #1
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
base: next
Are you sure you want to change the base?
Conversation
So far, a `Print` pass has been added every time the OpPassManager::nest() is called. The `Print` pass presented a good starting point for this issue, since it had implemented the basic building blocks for IR printing. What is currently missing is: - Configuring the IRPrintingConfig struct. At the moment, the IRPrintingConfig This, in turn, means that regardless of if there have been modifications to the IR or not the IR is *always* printed. The issue states that this should be configurable. I assume that these configurations should come from the Session/Context struct (although I don't think there's a 1 to 1 correspondance). - Realted to this, making printing granular is also missing. At the moment, the Print Pass is applied irregardless of the type. The issue also states that it should be able to *only print IR when a Pass modified the IR*. - One interesting note is that the PassManager has some functionality related to IR printing, although after some initial attempts I found it simpler to add that logic in the OpPassManager itself. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
What is currently missing is:
|
After exploring the codebase a bit further, I managed to find the So I'll move the logic to this trait. |
After exploring the codebase a bit further, I managed to find the PassInstrumentor vector, which holds a series of PassInstrumentations. These PassInstrumentations apply passes before and after other passes. This structure, I think, fits the IR printing issue better. So I'll move the logic to this struct. This reverts commit a0b54f1.
This is because both the Pass and OperationPass traits requiere it to execute. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
…untion." This reverts commit e9220bf.
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
…s for Print. which allows for all the cases to be executed and for the full structure of the IR to be printer. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Not yet used, but will determine how the Pass print is created. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
d16f53c
to
511dac2
Compare
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
…een changed. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
000eb60
to
9dc31b9
Compare
…y_when_modified Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
…IR modification with this enum. This is done in order to make IR modification more explicit and introduce it in the type system. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
1ef8b5d
to
56f6b6a
Compare
Now, the This value will later be used in the |
… to the PassInstrumentation implementation Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
The Pass implementation of the Print struct was removed since now the printing of the IR is done entirely within the If not removed, it could lead to a case where double printing occurs. For instance: if |
Added a These passes are received as a CLI argument from the They are now stored as enums, see: #1 (comment) |
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
46d67c1
to
b1a5722
Compare
59f4992
to
4333d35
Compare
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
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.
Left some comments, I'm not super knowledgeable on the codebase/compilers in general, so I mostly reviewed styling and overall logic/documentation.
hir/src/pass.rs
Outdated
/// Print IR regardless of which pass is executed. | ||
All, | ||
/// Only print IR if the pass's name is present in the vector. | ||
Certain(Vec<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.
nit: Given that in the example above the variants are named with the field used to filter. Should this variant be renamed to Identifier
or Name
?
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 think that this comment is related to this other comment.
The main issue is how much much do we stick to the original naming style.
hir/src/pass.rs
Outdated
pub fn new(config: &IRPrintingConfig) -> Option<Self> { | ||
if config.print_ir_after_all | ||
|| !config.print_ir_after_pass.is_empty() | ||
|| config.print_ir_after_modified | ||
{ | ||
Some(Self::default()) | ||
} else { | ||
None | ||
} |
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 find it strange that new
receives the config
but we only use it for a check. We really use the config in enable_ir_printing
, shouldn't this check be made there?
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 think having the logic separated could potentially lead to errors.
Here I moved all the logic to the new
method, what do you think?
hir/src/pass.rs
Outdated
Some(PassFilter::All) => true, | ||
Some(PassFilter::Certain(passes)) => passes.iter().any(|p| { | ||
if let Some(p_type) = pass.pass_id() { | ||
*p == p_type | ||
} else { | ||
false | ||
} | ||
}), |
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.
Perhaps not for this PR, but should this logic be handled by PassFilter
itself? It feels more at home there. The same for the check in print_ir
that decides wether or not to print for each OpFilter
.
result = Self::verify(&op, run_verifier_recursively); | ||
if let Err(verification_result) = Self::verify(&op, run_verifier_recursively) { | ||
result = result.map_err(|_| verification_result); | ||
} |
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.
Perhaps I'm not seeing something, but why did this change if Self::verify
wasn't modified? Couldn't we just do result = Self::verify(&op, run_verifier_recursively);
like before?
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 question! The verify function only checks if the Operation
follows its logical verification rules. It doesn't actually modify the structure.
Since the function doesn't modify the IR, its signature is Result<(), Report>
: It only returns a Report
if there's an issue with the modification.
That's why I used result = result.map_err
instead of result = result
} | ||
} | ||
|
||
// Return the pass result | ||
result | ||
result.map(|_| ()) |
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.
Same here, shouldn't we define result as Result<(), Err>
and just return result
?
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.
Similarly, we need to know if the passes have modified the IR to allow the possibility to filter the printing output.
That information however, is only used in the pass instrumentor which is called just above these lines, here:
https://github.com/lambdaclass/miden-compiler/blob/fabrizioorsi/print-ir-passmanager/hir/src/pass/manager.rs#L990-L991
But, after the pass is run, whether the IR has been modified or not is no longer used. The only thing that is used is if function returned a Err
, hence the map
.
/// Only print the IR if the pass modified the IR structure. If this flag is set, and no IR | ||
/// filter flag is; then the default behavior is to print the IR after every pass. |
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.
Phrasing seems confusing, it is my understanding that if it isn't set it will print depending on print_ir_after_all
. Maybe we should also mention how it behaves in relation to print_ir_after_all
because they seem like they are related.
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 talking with @lima-limon-inc I think I missunderstood the print_ir_after_all
flag. It doesn't mean "Print after all passes" but "Enable all passes to print", then the print_ir_after_modified
filters wich prints are shown.
Should we change the doc comments or flag names to reflect this?
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.
You brought up a great point, with the current naming style behavior is not only not obvious, but also a tad confusing.
I think the problem here is the same problem regarding the original naming convention in here: #1 (comment)
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
…e private. This was done in order to make the construction logic "coupled" to avoid decoupled logic problems. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Alos update documentaiton for enum PassFilter Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
a184948
to
3c951e7
Compare
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
NOTE: What should be the behavior when both Should the compiler crash and inform of this problem? |
hir/src/pass.rs
Outdated
|
||
/// A `Pass` which prints IR it is run on, based on provided configuration. | ||
#[derive(Default)] | ||
pub struct Print { | ||
filter: OpFilter, | ||
filter: Option<OpFilter>, | ||
pass_filter: Option<SelectedPasses>, |
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.
Let's rename this field to account for the enum name change
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 Ignacio! I actually thought the same thing but forgot to change it.
Changed here: 09aa5af
hir/src/pass.rs
Outdated
use crate::{ | ||
alloc::{string::String, vec::Vec}, | ||
EntityRef, Operation, OperationName, OperationRef, | ||
}; | ||
|
||
/// A `Pass` which prints IR it is run on, based on provided configuration. |
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.
This struct used to be more trivial but now we added 3 fields so it would be nice to briefly document how it works
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.
What do you think of this Ignacio: 3838fb6
Was something like that what you had in mind? Or were you thinking of a different approach?
hir/src/pass/pass.rs
Outdated
// NOTE: Not yet enabled. | ||
// InstructionScheduler, |
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 think we can remove this for now
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 thing Ignacio! Done here: 21aa490
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 documentation was a bit too CLI centric, I changed it here: 6522bae
… enum Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
a5b0619
to
3838fb6
Compare
What do you think of this addition: e0669e8 It basically checks if both the The output looks like this:
|
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
01dae73
to
6522bae
Compare
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
49382ed
to
ee46940
Compare
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
Now to compare pass equality, the `pass.name()` method is used. Signed-off-by: Tomas Fabrizio Orsi <tomas.orsi@lambdaclass.com>
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.NOTE: The name of the flags is still subject to change.
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 on-demand after a specific pass, it required manual code intervention & was not accessible to users of the compiler.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; hence I added it in these lines: https://github.com/lambdaclass/miden-compiler/pull/1/files#diff-537baa1178fde0adc5131faa9dda74d0fecbe4347403a71af509ffd401199001L11-R32Another thing that the issue required was the option to only print the IR after passes which modified itsstructured. That logic came in the form of the PostPassStatus enum, which is now returned by every
Pass::run_on_operation
function to indicate whether the Pass modified the IR or not.To actually enable IR printing, one has to pass the pass any one of the flags describe 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.