From d0549edecc38036d8f0e204e5c3f6e0bdc18841a Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Wed, 14 May 2025 17:56:46 -0300 Subject: [PATCH 01/22] Uncomment `#[ignore]` macro in `derived_op_verifier_test()` since test is fixed. Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 3fddbcfd..4e1ad97e 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -191,7 +191,6 @@ mod tests { )); } - #[ignore = "until https://github.com/0xMiden/compiler/issues/378 is fixed"] #[test] #[should_panic = "expected 'u32', got 'i64'"] fn derived_op_verifier_test() { From afd7d7949b121d31ca224aa35547f53c846d45df Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Tue, 13 May 2025 12:35:23 -0300 Subject: [PATCH 02/22] Enable operands_are_the_same_type validation Signed-off-by: Tomas Fabrizio Orsi --- hir/src/ir/traits/types.rs | 50 +++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/hir/src/ir/traits/types.rs b/hir/src/ir/traits/types.rs index 11305176..0309a607 100644 --- a/hir/src/ir/traits/types.rs +++ b/hir/src/ir/traits/types.rs @@ -24,10 +24,10 @@ derive! { pub trait SameTypeOperands {} verify { - fn operands_are_the_same_type(op: &Operation, _context: &Context) -> Result<(), Report> { + fn operands_are_the_same_type(op: &Operation, context: &Context) -> Result<(), Report> { let mut operands = op.operands().iter(); if let Some(first_operand) = operands.next() { - let (_expected_ty, _set_by) = { + let (expected_ty, set_by) = { let operand = first_operand.borrow(); let value = operand.value(); (value.ty().clone(), value.span()) @@ -36,29 +36,29 @@ derive! { for operand in operands { let operand = operand.borrow(); let value = operand.value(); - let _value_ty = value.ty(); - // if value_ty != &expected_ty { - // return Err(context - // .session - // .diagnostics - // .diagnostic(Severity::Error) - // .with_message(::alloc::format!("invalid operation {}", op.name())) - // .with_primary_label( - // op.span(), - // "this operation expects all operands to be of the same type" - // ) - // .with_secondary_label( - // set_by, - // "inferred the expected type from this value" - // ) - // .with_secondary_label( - // value.span(), - // "which differs from this value" - // ) - // .with_help(format!("expected '{expected_ty}', got '{value_ty}'")) - // .into_report() - // ); - // } + let value_ty = value.ty(); + if value_ty != &expected_ty { + return Err(context + .session() + .diagnostics + .diagnostic(Severity::Error) + .with_message(::alloc::format!("invalid operation {}", op.name())) + .with_primary_label( + op.span, + "this operation expects all operands to be of the same type" + ) + .with_secondary_label( + set_by, + "inferred the expected type from this value" + ) + .with_secondary_label( + value.span(), + "which differs from this value" + ) + .with_help(format!("expected '{expected_ty}', got '{value_ty}'")) + .into_report() + ); + } } } From da4e85a411ce900fc192cdcdcb219f464f6edf95 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Wed, 14 May 2025 12:29:53 -0300 Subject: [PATCH 03/22] WIP: First implementation for SameOperandsAndResultType validation Signed-off-by: Tomas Fabrizio Orsi --- hir/src/ir/traits/types.rs | 77 +++++++++++++++++++++++++++++++++++++- hir/src/ir/verifier.rs | 1 + 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/hir/src/ir/traits/types.rs b/hir/src/ir/traits/types.rs index 0309a607..82db70b2 100644 --- a/hir/src/ir/traits/types.rs +++ b/hir/src/ir/traits/types.rs @@ -4,7 +4,7 @@ use core::fmt; use midenc_hir_type::PointerType; use midenc_session::diagnostics::Severity; -use crate::{derive, Context, Op, Operation, Report, Type}; +use crate::{derive, ir::value::Value, Context, Op, Operation, Report, Type}; /// OpInterface to compute the return type(s) of an operation. pub trait InferTypeOpInterface: Op { @@ -74,6 +74,81 @@ derive! { /// as a super trait, check the operands using its implementation, and then check the results /// separately pub trait SameOperandsAndResultType {} + + verify { + fn operands_and_result_are_the_same_type(op: &Operation, context: &Context) -> Result<(), Report> { + // TODO: Later inherit from SameTypeOperands + let mut operands = op.operands().iter(); + if let Some(first_operand) = operands.next() { + let (expected_ty, set_by) = { + let operand = first_operand.borrow(); + let value = operand.value(); + (value.ty().clone(), value.span()) + }; + + let mut results = op.results().iter(); + + for operand in operands { + let operand = operand.borrow(); + let value = operand.value(); + let value_ty = value.ty(); + if value_ty != &expected_ty { + return Err(context + .session() + .diagnostics + .diagnostic(Severity::Error) + .with_message(::alloc::format!("invalid operation {}", op.name())) + .with_primary_label( + op.span, + "this operation expects all operands to be of the same type" + ) + .with_secondary_label( + set_by, + "inferred the expected type from this value" + ) + .with_secondary_label( + value.span(), + "which differs from this value" + ) + .with_help(format!("expected '{expected_ty}', got '{value_ty}'")) + .into_report() + ); + } + } + + for result in results { + let result = result.borrow(); + let value = result.as_value_ref().borrow(); + let result_ty = result.ty(); + + if result_ty != &expected_ty { + return Err(context + .session() + .diagnostics + .diagnostic(Severity::Error) + .with_message(::alloc::format!("invalid operation result {}", op.name())) + .with_primary_label( + op.span, + "this operation expects all operands to be of the same type" + ) + .with_secondary_label( + set_by, + "inferred the expected type from this value" + ) + .with_secondary_label( + value.span(), + "which differs from this value" + ) + .with_help(format!("expected '{expected_ty}', got '{result_ty}'")) + .into_report() + ); + } + }; + } + + Ok(()) + } + } } /// An operation trait that indicates it expects a variable number of operands, matching the given diff --git a/hir/src/ir/verifier.rs b/hir/src/ir/verifier.rs index 0edd7fec..42ad3b98 100644 --- a/hir/src/ir/verifier.rs +++ b/hir/src/ir/verifier.rs @@ -60,6 +60,7 @@ pub trait Verify { #[inline(always)] #[allow(unused_variables)] fn should_verify(&self, context: &Context) -> bool { + std::dbg!("Default"); true } /// Apply this verifier, but only if [Verify::should_verify] returns true. From 995a3e3887107035729f7b000ee6425ee86c253e Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Wed, 14 May 2025 16:17:25 -0300 Subject: [PATCH 04/22] Declare the InvalidOpsWithReturn struct and register it on the TestDialect Dialect Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 2 +- hir/src/dialects/test.rs | 1 + hir/src/dialects/test/ops/binary.rs | 16 ++++++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 4e1ad97e..44a80abd 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -136,7 +136,7 @@ mod tests { use crate::{ attributes::Overflow, - dialects::test::{self, Add}, + dialects::test::{self, Add, InvalidOpsWithReturn}, Builder, BuilderExt, Context, Op, Operation, Report, Spanned, }; diff --git a/hir/src/dialects/test.rs b/hir/src/dialects/test.rs index 40b40b0e..64d698f2 100644 --- a/hir/src/dialects/test.rs +++ b/hir/src/dialects/test.rs @@ -115,6 +115,7 @@ impl DialectRegistration for TestDialect { fn register_operations(info: &mut DialectInfo) { info.register_operation::(); + info.register_operation::(); info.register_operation::(); info.register_operation::(); info.register_operation::(); diff --git a/hir/src/dialects/test/ops/binary.rs b/hir/src/dialects/test/ops/binary.rs index 534b1be7..15ed2280 100644 --- a/hir/src/dialects/test/ops/binary.rs +++ b/hir/src/dialects/test/ops/binary.rs @@ -74,3 +74,19 @@ impl InferTypeOpInterface for Shl { Ok(()) } } + +/// Invalid operation that breaks the SameOperandsAndResultType trait +#[operation( + dialect = TestDialect, + traits(BinaryOp, SameTypeOperands, SameOperandsAndResultType), +)] +pub struct InvalidOpsWithReturn { + #[operand] + lhs: AnyInteger, + #[operand] + rhs: AnyInteger, + #[result] + result: AnyUnsignedInteger, + #[attr] + overflow: Overflow, +} From 38e85fa1b56208f35a0a7c27189054dd9cebf562 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Wed, 14 May 2025 17:53:27 -0300 Subject: [PATCH 05/22] Implement SameOperandsAndResultType test. Signed-off-by: Tomas Fabrizio Orsi Signed-off-by: Tomas Fabrizio Orsi Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 24 ++++++++++++++++++++++++ hir/src/dialects/test/ops/binary.rs | 12 +++++++++--- hir/src/ir/traits/types.rs | 2 +- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 44a80abd..1b7df045 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -211,4 +211,28 @@ mod tests { let op = op_builder(lhs, invalid_rhs, Overflow::Wrapping); let _op = op.unwrap(); } + + /// Fails if [`InvalidOpsWithReturn`] is created successfully. [`InvalidOpsWithReturn`] is a + /// struct that has differing types in its result and arguments, despite implementing the + /// [`SameOperandsAndResultType`] trait. + #[test] + #[should_panic = "expected 'i32', got 'u64'"] + fn same_operands_and_result_type_verifier_test() { + use crate::{SourceSpan, Type}; + + let context = Rc::new(Context::default()); + let block = context.create_block_with_params([Type::I32, Type::I32]); + let (lhs, rhs) = { + let block = block.borrow(); + let lhs = block.get_argument(0).upcast::(); + let rhs = block.get_argument(1).upcast::(); + (lhs, rhs) + }; + let mut builder = context.builder(); + builder.set_insertion_point_to_end(block); + // Try to create instance of AddOp with mismatched operand types + let op_builder = builder.create::(SourceSpan::default()); + let op = op_builder(lhs, rhs); + let _op = op.unwrap(); + } } diff --git a/hir/src/dialects/test/ops/binary.rs b/hir/src/dialects/test/ops/binary.rs index 15ed2280..e48954c0 100644 --- a/hir/src/dialects/test/ops/binary.rs +++ b/hir/src/dialects/test/ops/binary.rs @@ -75,10 +75,11 @@ impl InferTypeOpInterface for Shl { } } -/// Invalid operation that breaks the SameOperandsAndResultType trait +/// Invalid operation that breaks the SameOperandsAndResultType trait (used for testing). #[operation( dialect = TestDialect, traits(BinaryOp, SameTypeOperands, SameOperandsAndResultType), + implements(InferTypeOpInterface) )] pub struct InvalidOpsWithReturn { #[operand] @@ -87,6 +88,11 @@ pub struct InvalidOpsWithReturn { rhs: AnyInteger, #[result] result: AnyUnsignedInteger, - #[attr] - overflow: Overflow, +} + +impl InferTypeOpInterface for InvalidOpsWithReturn { + fn infer_return_types(&mut self, _context: &Context) -> Result<(), Report> { + self.result_mut().set_type(Type::U64); + Ok(()) + } } diff --git a/hir/src/ir/traits/types.rs b/hir/src/ir/traits/types.rs index 82db70b2..76cdbba4 100644 --- a/hir/src/ir/traits/types.rs +++ b/hir/src/ir/traits/types.rs @@ -86,7 +86,7 @@ derive! { (value.ty().clone(), value.span()) }; - let mut results = op.results().iter(); + let results = op.results().iter(); for operand in operands { let operand = operand.borrow(); From 8ac10e11058e3e14a3557b3038ab56d10cee3c8d Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 09:45:47 -0300 Subject: [PATCH 06/22] Add $ParentTrait pattern to verify macro + Add SameTypeOperands as a explicit dependency Signed-off-by: Tomas Fabrizio Orsi --- dialects/arith/src/ops/unary.rs | 14 +++--- dialects/hir/src/ops/primop.rs | 2 +- dialects/hir/src/ops/spills.rs | 4 +- hir/src/derive.rs | 88 +++++++++++++++++++++++++++++++++ hir/src/ir/traits/types.rs | 2 +- 5 files changed, 99 insertions(+), 11 deletions(-) diff --git a/dialects/arith/src/ops/unary.rs b/dialects/arith/src/ops/unary.rs index 373daf03..4d6255ea 100644 --- a/dialects/arith/src/ops/unary.rs +++ b/dialects/arith/src/ops/unary.rs @@ -28,7 +28,7 @@ macro_rules! infer_return_ty_for_unary_op { /// Increment #[operation ( dialect = ArithDialect, - traits(UnaryOp, SameOperandsAndResultType), + traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] pub struct Incr { @@ -44,7 +44,7 @@ has_no_effects!(Incr); /// Negation #[operation ( dialect = ArithDialect, - traits(UnaryOp, SameOperandsAndResultType), + traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] pub struct Neg { @@ -60,7 +60,7 @@ has_no_effects!(Neg); /// Modular inverse #[operation ( dialect = ArithDialect, - traits(UnaryOp, SameOperandsAndResultType), + traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] pub struct Inv { @@ -76,7 +76,7 @@ has_no_effects!(Inv); /// log2(operand) #[operation ( dialect = ArithDialect, - traits(UnaryOp, SameOperandsAndResultType), + traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] pub struct Ilog2 { @@ -92,7 +92,7 @@ has_no_effects!(Ilog2); /// pow2(operand) #[operation ( dialect = ArithDialect, - traits(UnaryOp, SameOperandsAndResultType), + traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] pub struct Pow2 { @@ -108,7 +108,7 @@ has_no_effects!(Pow2); /// Logical NOT #[operation ( dialect = ArithDialect, - traits(UnaryOp, SameOperandsAndResultType), + traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] @@ -125,7 +125,7 @@ has_no_effects!(Not); /// Bitwise NOT #[operation ( dialect = ArithDialect, - traits(UnaryOp, SameOperandsAndResultType), + traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] pub struct Bnot { diff --git a/dialects/hir/src/ops/primop.rs b/dialects/hir/src/ops/primop.rs index ad89c410..7d9c72a6 100644 --- a/dialects/hir/src/ops/primop.rs +++ b/dialects/hir/src/ops/primop.rs @@ -4,7 +4,7 @@ use crate::HirDialect; #[operation( dialect = HirDialect, - traits(SameOperandsAndResultType), + traits(SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface) )] pub struct MemGrow { diff --git a/dialects/hir/src/ops/spills.rs b/dialects/hir/src/ops/spills.rs index bd2eb93a..b684ef3f 100644 --- a/dialects/hir/src/ops/spills.rs +++ b/dialects/hir/src/ops/spills.rs @@ -5,7 +5,7 @@ use crate::HirDialect; #[operation( dialect = HirDialect, - traits(SameOperandsAndResultType), + traits(SameTypeOperands, SameOperandsAndResultType), implements(MemoryEffectOpInterface, SpillLike) )] pub struct Spill { @@ -34,7 +34,7 @@ impl EffectOpInterface for Spill { #[operation( dialect = HirDialect, - traits(SameOperandsAndResultType), + traits(SameTypeOperands, SameOperandsAndResultType), implements(InferTypeOpInterface, MemoryEffectOpInterface, ReloadLike) )] pub struct Reload { diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 1b7df045..384fc7b9 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -37,6 +37,46 @@ macro_rules! derive { $($t)* }; + ( + $(#[$outer:meta])* + $vis:vis trait $OpTrait:ident : $ParentTrait:ident { + $( + $OpTraitItem:item + )* + } + + verify { + $( + fn $verify_fn:ident($op:ident: &$OperationPath:path, $ctx:ident: &$ContextPath:path) -> $VerifyResult:ty $verify:block + )+ + } + + $($t:tt)* + ) => { + $crate::__derive_op_trait! { + $(#[$outer])* + $vis trait $OpTrait : $ParentTrait { + $( + $OpTraitItem:item + )* + } + + verify { + $( + fn $verify_fn($op: &$OperationPath, $ctx: &$ContextPath) -> $VerifyResult $verify + )* + } + } + + $($t)* + }; + + + + + + + ( $(#[$outer:meta])* $vis:vis trait $OpTrait:ident { @@ -111,6 +151,54 @@ macro_rules! __derive_op_trait { } }; + ( + $(#[$outer:meta])* + $vis:vis trait $OpTrait:ident : $ParentTrait:ident { + $( + $OpTraitItem:item + )* + } + + verify { + $( + fn $verify_fn:ident($op:ident: &$OperationPath:path, $ctx:ident: &$ContextPath:path) -> $VerifyResult:ty $verify:block + )+ + } + ) => { + $(#[$outer])* + $vis trait $OpTrait : $ParentTrait { + $( + $OpTraitItem + )* + } + + impl $crate::Verify for T { + #[inline] + fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { + <$crate::Operation as $crate::Verify>::verify(self.as_operation(), context) + } + } + + impl $crate::Verify for $crate::Operation { + fn should_verify(&self, _context: &$crate::Context) -> bool { + self.implements::() + } + + fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { + $( + #[inline] + fn $verify_fn($op: &$OperationPath, $ctx: &$ContextPath) -> $VerifyResult $verify + )* + + $( + $verify_fn(self, context)?; + )* + + Ok(()) + } + } + }; + ( $(#[$outer:meta])* $vis:vis trait $OpTrait:ident { diff --git a/hir/src/ir/traits/types.rs b/hir/src/ir/traits/types.rs index 76cdbba4..bc6f220a 100644 --- a/hir/src/ir/traits/types.rs +++ b/hir/src/ir/traits/types.rs @@ -73,7 +73,7 @@ derive! { /// TODO(pauls): Implement verification for this. Ideally we could require `SameTypeOperands` /// as a super trait, check the operands using its implementation, and then check the results /// separately - pub trait SameOperandsAndResultType {} + pub trait SameOperandsAndResultType: SameTypeOperands {} verify { fn operands_and_result_are_the_same_type(op: &Operation, context: &Context) -> Result<(), Report> { From 3723b756b40a3af0d54fe6e40e15b443ec2a3f4a Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 10:52:35 -0300 Subject: [PATCH 07/22] Call parent trait verify function before OPTrait verify Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 3 +++ hir/src/ir/verifier.rs | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 384fc7b9..a8af573d 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -175,6 +175,7 @@ macro_rules! __derive_op_trait { impl $crate::Verify for T { #[inline] fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { + <$crate::Operation as $crate::Verify>::verify(self.as_operation(), context)?; <$crate::Operation as $crate::Verify>::verify(self.as_operation(), context) } } @@ -182,6 +183,8 @@ macro_rules! __derive_op_trait { impl $crate::Verify for $crate::Operation { fn should_verify(&self, _context: &$crate::Context) -> bool { self.implements::() + && + self.implements::() } fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { diff --git a/hir/src/ir/verifier.rs b/hir/src/ir/verifier.rs index 42ad3b98..0edd7fec 100644 --- a/hir/src/ir/verifier.rs +++ b/hir/src/ir/verifier.rs @@ -60,7 +60,6 @@ pub trait Verify { #[inline(always)] #[allow(unused_variables)] fn should_verify(&self, context: &Context) -> bool { - std::dbg!("Default"); true } /// Apply this verifier, but only if [Verify::should_verify] returns true. From 0e4a259b6f4c2c41fade94bb07d8d1f3a115e0b4 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 11:09:08 -0300 Subject: [PATCH 08/22] Remove operand type check from SameOperandsAndResultType Signed-off-by: Tomas Fabrizio Orsi --- hir/src/ir/traits/types.rs | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/hir/src/ir/traits/types.rs b/hir/src/ir/traits/types.rs index bc6f220a..1aab299a 100644 --- a/hir/src/ir/traits/types.rs +++ b/hir/src/ir/traits/types.rs @@ -88,34 +88,6 @@ derive! { let results = op.results().iter(); - for operand in operands { - let operand = operand.borrow(); - let value = operand.value(); - let value_ty = value.ty(); - if value_ty != &expected_ty { - return Err(context - .session() - .diagnostics - .diagnostic(Severity::Error) - .with_message(::alloc::format!("invalid operation {}", op.name())) - .with_primary_label( - op.span, - "this operation expects all operands to be of the same type" - ) - .with_secondary_label( - set_by, - "inferred the expected type from this value" - ) - .with_secondary_label( - value.span(), - "which differs from this value" - ) - .with_help(format!("expected '{expected_ty}', got '{value_ty}'")) - .into_report() - ); - } - } - for result in results { let result = result.borrow(); let value = result.as_value_ref().borrow(); From 11aee8869637b30cd0958a24ac725169be60caee Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 12:28:08 -0300 Subject: [PATCH 09/22] Make it possible to inherit an arbitrary amount of parent traits (currently comma separated) Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index a8af573d..1130c6bf 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -39,7 +39,7 @@ macro_rules! derive { ( $(#[$outer:meta])* - $vis:vis trait $OpTrait:ident : $ParentTrait:ident { + $vis:vis trait $OpTrait:ident : $( $ParentTrait:ident ),* $(,)? { $( $OpTraitItem:item )* @@ -55,7 +55,7 @@ macro_rules! derive { ) => { $crate::__derive_op_trait! { $(#[$outer])* - $vis trait $OpTrait : $ParentTrait { + $vis trait $OpTrait : $( $ParentTrait , )* { $( $OpTraitItem:item )* @@ -153,7 +153,7 @@ macro_rules! __derive_op_trait { ( $(#[$outer:meta])* - $vis:vis trait $OpTrait:ident : $ParentTrait:ident { + $vis:vis trait $OpTrait:ident : $( $ParentTrait:ident ),* $(,)? { $( $OpTraitItem:item )* @@ -166,7 +166,7 @@ macro_rules! __derive_op_trait { } ) => { $(#[$outer])* - $vis trait $OpTrait : $ParentTrait { + $vis trait $OpTrait : $( $ParentTrait + )* { $( $OpTraitItem )* @@ -175,7 +175,9 @@ macro_rules! __derive_op_trait { impl $crate::Verify for T { #[inline] fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { + $( <$crate::Operation as $crate::Verify>::verify(self.as_operation(), context)?; + )* <$crate::Operation as $crate::Verify>::verify(self.as_operation(), context) } } @@ -183,8 +185,10 @@ macro_rules! __derive_op_trait { impl $crate::Verify for $crate::Operation { fn should_verify(&self, _context: &$crate::Context) -> bool { self.implements::() + $( && - self.implements::() + self.implements::() + )* } fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { From 5cfee48c0cdacbdd9027fd9e2b2f811d0f4678b0 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 14:14:00 -0300 Subject: [PATCH 10/22] Update diagnostic description Signed-off-by: Tomas Fabrizio Orsi --- hir/src/ir/traits/types.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hir/src/ir/traits/types.rs b/hir/src/ir/traits/types.rs index 1aab299a..129a72e0 100644 --- a/hir/src/ir/traits/types.rs +++ b/hir/src/ir/traits/types.rs @@ -77,7 +77,6 @@ derive! { verify { fn operands_and_result_are_the_same_type(op: &Operation, context: &Context) -> Result<(), Report> { - // TODO: Later inherit from SameTypeOperands let mut operands = op.operands().iter(); if let Some(first_operand) = operands.next() { let (expected_ty, set_by) = { @@ -101,7 +100,7 @@ derive! { .with_message(::alloc::format!("invalid operation result {}", op.name())) .with_primary_label( op.span, - "this operation expects all operands to be of the same type" + "this operation expects the operands and the results to be of the same type" ) .with_secondary_label( set_by, From b81979f53d979a42b9d00ad06fa09d0a3ac5a329 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 14:50:07 -0300 Subject: [PATCH 11/22] Add documentation to SameOperandsAndResultType and derive macro Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 5 +++++ hir/src/ir/traits/types.rs | 17 +++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 1130c6bf..f64cf062 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -1,6 +1,11 @@ pub use midenc_hir_macros::operation; /// This macro is used to generate the boilerplate for operation trait implementations. +/// Super traits have to be declared as a comma separated list of traits, instead of the traditional +/// "+" separated list of traits. +/// Example: +/// +/// pub trait SomeTrait: SuperTraitA, SuperTraitB {} #[macro_export] macro_rules! derive { ( diff --git a/hir/src/ir/traits/types.rs b/hir/src/ir/traits/types.rs index 129a72e0..ff1462e4 100644 --- a/hir/src/ir/traits/types.rs +++ b/hir/src/ir/traits/types.rs @@ -68,11 +68,20 @@ derive! { } derive! { - /// Op expects all operands and results to be of the same type + /// Op expects all operands and results to be of the same type. + /// NOTE: Operations that implements this trait must also explicitely implement + /// [`SameTypeOperands`]. This can be achieved by using the "traits" filed in the [#operation] + /// macro. + /// Like so: /// - /// TODO(pauls): Implement verification for this. Ideally we could require `SameTypeOperands` - /// as a super trait, check the operands using its implementation, and then check the results - /// separately + /// #[operation ( + /// dialect = ArithDialect, + /// traits(UnaryOp, SameTypeOperands, SameOperandsAndResultType), + /// implements(InferTypeOpInterface, MemoryEffectOpInterface) + /// )] + /// pub struct SomeOp { + /// (...) + /// } pub trait SameOperandsAndResultType: SameTypeOperands {} verify { From 1a214fe78eb68dd41d13238f0fd5ca7405f4f1fd Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 14:52:05 -0300 Subject: [PATCH 12/22] Remove additional spaces Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index f64cf062..c4992c9f 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -76,12 +76,6 @@ macro_rules! derive { $($t)* }; - - - - - - ( $(#[$outer:meta])* $vis:vis trait $OpTrait:ident { From 62899533621ac1c6af5d28cadb14d6013fce3dd2 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 15:02:00 -0300 Subject: [PATCH 13/22] Remove the additional pattern declaration, leave an optional semi-colon which matches both patterns Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 86 ++--------------------------------------------- 1 file changed, 2 insertions(+), 84 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index c4992c9f..7830e77a 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -10,41 +10,7 @@ pub use midenc_hir_macros::operation; macro_rules! derive { ( $(#[$outer:meta])* - $vis:vis trait $OpTrait:ident { - $( - $OpTraitItem:item - )* - } - - verify { - $( - fn $verify_fn:ident($op:ident: &$OperationPath:path, $ctx:ident: &$ContextPath:path) -> $VerifyResult:ty $verify:block - )+ - } - - $($t:tt)* - ) => { - $crate::__derive_op_trait! { - $(#[$outer])* - $vis trait $OpTrait { - $( - $OpTraitItem:item - )* - } - - verify { - $( - fn $verify_fn($op: &$OperationPath, $ctx: &$ContextPath) -> $VerifyResult $verify - )* - } - } - - $($t)* - }; - - ( - $(#[$outer:meta])* - $vis:vis trait $OpTrait:ident : $( $ParentTrait:ident ),* $(,)? { + $vis:vis trait $OpTrait:ident $(:)? $( $ParentTrait:ident ),* $(,)? { $( $OpTraitItem:item )* @@ -104,55 +70,7 @@ macro_rules! derive { macro_rules! __derive_op_trait { ( $(#[$outer:meta])* - $vis:vis trait $OpTrait:ident { - $( - $OpTraitItem:item - )* - } - - verify { - $( - fn $verify_fn:ident($op:ident: &$OperationPath:path, $ctx:ident: &$ContextPath:path) -> $VerifyResult:ty $verify:block - )+ - } - ) => { - $(#[$outer])* - $vis trait $OpTrait { - $( - $OpTraitItem - )* - } - - impl $crate::Verify for T { - #[inline] - fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { - <$crate::Operation as $crate::Verify>::verify(self.as_operation(), context) - } - } - - impl $crate::Verify for $crate::Operation { - fn should_verify(&self, _context: &$crate::Context) -> bool { - self.implements::() - } - - fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { - $( - #[inline] - fn $verify_fn($op: &$OperationPath, $ctx: &$ContextPath) -> $VerifyResult $verify - )* - - $( - $verify_fn(self, context)?; - )* - - Ok(()) - } - } - }; - - ( - $(#[$outer:meta])* - $vis:vis trait $OpTrait:ident : $( $ParentTrait:ident ),* $(,)? { + $vis:vis trait $OpTrait:ident $(:)? $( $ParentTrait:ident ),* $(,)? { $( $OpTraitItem:item )* From 919a342d55b43bd04a1eba0733c5c4c78b82b415 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 15:05:54 -0300 Subject: [PATCH 14/22] Verify the parent traits before the operation trait. Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 7830e77a..d350225a 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -101,11 +101,11 @@ macro_rules! __derive_op_trait { impl $crate::Verify for $crate::Operation { fn should_verify(&self, _context: &$crate::Context) -> bool { - self.implements::() $( - && self.implements::() + && )* + self.implements::() } fn verify(&self, context: &$crate::Context) -> Result<(), $crate::Report> { From 9917e95fb73ae9a2f1a96ab352857281770c1069 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 16:49:53 -0300 Subject: [PATCH 15/22] CHECKPOINT Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 26 ++++++++++++++++++++++---- hir/src/ir/operation/builder.rs | 8 ++++---- hir/src/lib.rs | 2 +- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index d350225a..471e8ee5 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -148,8 +148,12 @@ mod tests { use crate::{ attributes::Overflow, - dialects::test::{self, Add, InvalidOpsWithReturn}, - Builder, BuilderExt, Context, Op, Operation, Report, Spanned, + dialects::{ + builtin, + test::{self, Add, InvalidOpsWithReturn}, + }, + pass::{Nesting, PassManager}, + BlockArgument, Builder, BuilderExt, Context, Op, Operation, Report, Spanned, ValueId, }; derive! { @@ -209,19 +213,33 @@ mod tests { use crate::{SourceSpan, Type}; let context = Rc::new(Context::default()); + let block = context.create_block_with_params([Type::U32, Type::I64]); + + context.get_or_register_dialect::(); + context.registered_dialects(); + + // let lhs = + // BlockArgument::new(SourceSpan::default(), ValueId::from_u32(0), Type::U32, None, 0); + let (lhs, invalid_rhs) = { let block = block.borrow(); let lhs = block.get_argument(0).upcast::(); let rhs = block.get_argument(1).upcast::(); (lhs, rhs) }; - let mut builder = context.builder(); + + let mut builder = context.clone().builder(); builder.set_insertion_point_to_end(block); // Try to create instance of AddOp with mismatched operand types let op_builder = builder.create::(SourceSpan::default()); let op = op_builder(lhs, invalid_rhs, Overflow::Wrapping); - let _op = op.unwrap(); + let op = op.unwrap(); + + // Construct a pass manager with the default pass pipeline + let mut pm = PassManager::on::(context.clone(), Nesting::Implicit); + // Run pass pipeline + pm.run(op.as_operation_ref()).unwrap(); } /// Fails if [`InvalidOpsWithReturn`] is created successfully. [`InvalidOpsWithReturn`] is a diff --git a/hir/src/ir/operation/builder.rs b/hir/src/ir/operation/builder.rs index 251de660..67a6b390 100644 --- a/hir/src/ir/operation/builder.rs +++ b/hir/src/ir/operation/builder.rs @@ -257,10 +257,10 @@ where }; // Run op-specific verification - { - let op: super::EntityRef = op.borrow(); - op.verify(self.builder.context())?; - } + // { + // let op: super::EntityRef = op.borrow(); + // op.verify(self.builder.context())?; + // } // Insert op at current insertion point, if set if self.builder.insertion_point().is_valid() { diff --git a/hir/src/lib.rs b/hir/src/lib.rs index 51f0af05..d5bcac6d 100644 --- a/hir/src/lib.rs +++ b/hir/src/lib.rs @@ -41,7 +41,7 @@ // Some of the above features require us to disable these warnings #![allow(incomplete_features)] #![allow(internal_features)] -#![deny(warnings)] +// #![deny(warnings)] extern crate alloc; From 3aadadf05eaffc4ee488ca8bc168f29a9b61fa44 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 17:25:40 -0300 Subject: [PATCH 16/22] Momentarily verify early Signed-off-by: Tomas Fabrizio Orsi --- hir/src/pass/manager.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hir/src/pass/manager.rs b/hir/src/pass/manager.rs index c0e2b9ad..3edc705f 100644 --- a/hir/src/pass/manager.rs +++ b/hir/src/pass/manager.rs @@ -845,6 +845,9 @@ impl OpToOpPassAdaptor { instrumentor: Option>, parent_info: Option<&PipelineParentInfo>, ) -> Result<(), Report> { + + Self::verify(&op, true)?; + assert!( instrumentor.is_none() || parent_info.is_some(), "expected parent info if instrumentor is provided" From 68df5f40d92f6d2e3b89bdb5d74f33a44e027040 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 17:38:22 -0300 Subject: [PATCH 17/22] Add pass manager in test Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 9 +++++++-- hir/src/pass/manager.rs | 4 +++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 471e8ee5..f8a84304 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -258,11 +258,16 @@ mod tests { let rhs = block.get_argument(1).upcast::(); (lhs, rhs) }; - let mut builder = context.builder(); + let mut builder = context.clone().builder(); builder.set_insertion_point_to_end(block); // Try to create instance of AddOp with mismatched operand types let op_builder = builder.create::(SourceSpan::default()); let op = op_builder(lhs, rhs); - let _op = op.unwrap(); + let op = op.unwrap(); + + // Construct a pass manager with the default pass pipeline + let mut pm = PassManager::on::(context.clone(), Nesting::Implicit); + // Run pass pipeline + pm.run(op.as_operation_ref()).unwrap(); } } diff --git a/hir/src/pass/manager.rs b/hir/src/pass/manager.rs index 3edc705f..4cc46730 100644 --- a/hir/src/pass/manager.rs +++ b/hir/src/pass/manager.rs @@ -846,7 +846,9 @@ impl OpToOpPassAdaptor { parent_info: Option<&PipelineParentInfo>, ) -> Result<(), Report> { - Self::verify(&op, true)?; + if verify { + Self::verify(&op, true)?; + } assert!( instrumentor.is_none() || parent_info.is_some(), From 2bd389ed7cae83764e32b0825c7deb4c511d7de6 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 17:54:19 -0300 Subject: [PATCH 18/22] Add a comment explaining why we do an initial verification Signed-off-by: Tomas Fabrizio Orsi --- hir/src/pass/manager.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hir/src/pass/manager.rs b/hir/src/pass/manager.rs index 4cc46730..5af1eda8 100644 --- a/hir/src/pass/manager.rs +++ b/hir/src/pass/manager.rs @@ -847,6 +847,8 @@ impl OpToOpPassAdaptor { ) -> Result<(), Report> { if verify { + // We run an initial recursive verification, since this is potentially the first + // verification done in the operations Self::verify(&op, true)?; } From 634245ba6c51b33b5e3f1194ec9df80193646772 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 17:56:59 -0300 Subject: [PATCH 19/22] Delete op.verification all-together Signed-off-by: Tomas Fabrizio Orsi --- hir/src/ir/operation/builder.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hir/src/ir/operation/builder.rs b/hir/src/ir/operation/builder.rs index 67a6b390..dbd5b66e 100644 --- a/hir/src/ir/operation/builder.rs +++ b/hir/src/ir/operation/builder.rs @@ -256,12 +256,6 @@ where unsafe { UnsafeIntrusiveEntityRef::from_raw(op.container().cast()) } }; - // Run op-specific verification - // { - // let op: super::EntityRef = op.borrow(); - // op.verify(self.builder.context())?; - // } - // Insert op at current insertion point, if set if self.builder.insertion_point().is_valid() { self.builder.insert(self.op); From 735e322e668fe67b8379cc21b04fe2c4e79496fa Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 17:59:47 -0300 Subject: [PATCH 20/22] Delete commented out declaration in test Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index f8a84304..72d87ac0 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -219,9 +219,6 @@ mod tests { context.get_or_register_dialect::(); context.registered_dialects(); - // let lhs = - // BlockArgument::new(SourceSpan::default(), ValueId::from_u32(0), Type::U32, None, 0); - let (lhs, invalid_rhs) = { let block = block.borrow(); let lhs = block.get_argument(0).upcast::(); From 5a35ca45c7e5d30fc4920f8b578f6f1b209d1fc6 Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Thu, 15 May 2025 18:03:22 -0300 Subject: [PATCH 21/22] Remove un-used imports Signed-off-by: Tomas Fabrizio Orsi --- hir/src/derive.rs | 7 ++----- hir/src/lib.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/hir/src/derive.rs b/hir/src/derive.rs index 72d87ac0..71f059c2 100644 --- a/hir/src/derive.rs +++ b/hir/src/derive.rs @@ -148,12 +148,9 @@ mod tests { use crate::{ attributes::Overflow, - dialects::{ - builtin, - test::{self, Add, InvalidOpsWithReturn}, - }, + dialects::test::{self, Add, InvalidOpsWithReturn}, pass::{Nesting, PassManager}, - BlockArgument, Builder, BuilderExt, Context, Op, Operation, Report, Spanned, ValueId, + Builder, BuilderExt, Context, Op, Operation, Report, Spanned, }; derive! { diff --git a/hir/src/lib.rs b/hir/src/lib.rs index d5bcac6d..51f0af05 100644 --- a/hir/src/lib.rs +++ b/hir/src/lib.rs @@ -41,7 +41,7 @@ // Some of the above features require us to disable these warnings #![allow(incomplete_features)] #![allow(internal_features)] -// #![deny(warnings)] +#![deny(warnings)] extern crate alloc; From 4baa815d91bc127230751abde7fde3c40fedad5d Mon Sep 17 00:00:00 2001 From: Tomas Fabrizio Orsi Date: Fri, 16 May 2025 11:55:31 -0300 Subject: [PATCH 22/22] Update comment regarding Operation::verify Signed-off-by: Tomas Fabrizio Orsi --- hir/src/pass/manager.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hir/src/pass/manager.rs b/hir/src/pass/manager.rs index 5af1eda8..5ab019b0 100644 --- a/hir/src/pass/manager.rs +++ b/hir/src/pass/manager.rs @@ -847,8 +847,8 @@ impl OpToOpPassAdaptor { ) -> Result<(), Report> { if verify { - // We run an initial recursive verification, since this is potentially the first - // verification done in the operations + // We run an initial recursive verification, since this is the first verification done + // to the operations Self::verify(&op, true)?; }