Skip to content

Remove verify from OperationBuilder and SameOperandsAndResultType verification implementation #3

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

Draft
wants to merge 22 commits into
base: next
Choose a base branch
from

Conversation

lima-limon-inc
Copy link

@lima-limon-inc lima-limon-inc commented May 14, 2025

Closes # issue nm

The issue states that currently Operation::verify is called inside the OperationBuilder which is called before operations are added to blocks, which could lead failure in operation verification. Because of this, SameTypeOperands' verification function had been commented out.

This PR removes the call to Operation::verify from the OperationBuilder and moves it to the beggining of PassManager::run. Operation::verify is already called inside the PassManager::run (more precisely, inside the OpToOpPassAdaptor::run_pipeline), however an earlier verify call was added to "compensate" the removal and assure Operation integrity before applying passes. Now, the PassManger is responsible for Operation integrity verification, which is done before passes are applied and after each pass applies its modifications.

With the call to verify in the OperationBuilder now removed, SameTypeOperands' verification function was re-enabled and so was its test (derived_op_verifier_test).

Additionally, an implementation for SameOperandsAndResultType's verification function was added (which was marked here as a TODO-item). Following the comment's requirements, SameTypeOperands required as a super-trait.

To support this, the derive! macro was expanded to support super-traits. The macro supports an arbitrary amount of traits; but it is important to note that the traits must be comma separated instead of the usual "+" separated format. As far as I am aware, it is not possible to use "+" as a separator inside a macro_rules! context.

With the super-trait now added, operations that implemented SameOperandsAndResultType but did not implement SameTypeOperands were updated to now explicitely implement both. This was done explicitely in order to replicate Rust's #[derive] macro's behavior of explicitely declaring all derive traits.

Finally, a unit test was added to test SameOperandsAndResultType's verify function. For this, a test operation InvalidOpsWithReturn was added and registered in the test dialect.

Side note: I tried to keep the SameOperandsAndResultType's doc comment relatively short in order for it to show up in its entirety when a compilation error shows up, it is currently displayed like so:

72  | / derive! {
73  | |     /// Op expects all operands and results to be of the same type.
74  | |     /// NOTE: Operations that implements this trait must also explicitely implement
75  | |     /// [`SameTypeOperands`]. This can be achieved by using the "traits" filed in the [#operation]
...   |
87  | |     pub trait SameOperandsAndResultType: SameTypeOperands {}
    | |               ------------------------- required by a bound in this trait

@lima-limon-inc lima-limon-inc self-assigned this May 14, 2025
@lima-limon-inc lima-limon-inc changed the title WIP: Remove verify from op builder + SameTypeOperands implementation WIP: Remove verify from op builder + SameOperandsAndResultType implementation May 14, 2025
@lima-limon-inc lima-limon-inc force-pushed the fabrizioorsi/rm-verify-op-builder branch 2 times, most recently from ac061f1 to 8dc03a9 Compare May 14, 2025 20:56
@lima-limon-inc lima-limon-inc force-pushed the fabrizioorsi/rm-verify-op-builder branch 3 times, most recently from 6fc9bea to 5959688 Compare May 15, 2025 12:47
@lima-limon-inc lima-limon-inc changed the title WIP: Remove verify from op builder + SameOperandsAndResultType implementation Remove verify from op builder + SameOperandsAndResultType implementation May 16, 2025
@lima-limon-inc lima-limon-inc changed the title Remove verify from op builder + SameOperandsAndResultType implementation Remove verify from OperationBuilder + SameOperandsAndResultType implementation May 16, 2025
@lima-limon-inc lima-limon-inc changed the title Remove verify from OperationBuilder + SameOperandsAndResultType implementation Remove verify from OperationBuilder and SameOperandsAndResultType verification implementation May 16, 2025
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>

Signed-off-by: Tomas Fabrizio Orsi <[email protected]>

Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
…explicit dependency

Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
…rently comma separated)

Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
…on which matches both patterns

Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
@lima-limon-inc lima-limon-inc force-pushed the fabrizioorsi/rm-verify-op-builder branch from e007d01 to 4baa815 Compare May 16, 2025 18:43
@lima-limon-inc
Copy link
Author

Just Rebased

@lima-limon-inc
Copy link
Author

lima-limon-inc commented May 16, 2025

After re-reading the documentation, this PR should also come with a documentation change; subsection "Validation" from the "Builder" section.

Maybe it could be done after a round of reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant