Skip to content
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

Core: Add IsTerminator trait #1080

Merged
merged 30 commits into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8571d56
Add IsTerminator trait
compor Jun 5, 2023
32d4719
Add terminator check in verify method for operation
compor Jun 5, 2023
379168f
Add tests for terminator sanity checking
compor Jun 5, 2023
42c2991
Add tests for IsTerminator trait
compor Jun 5, 2023
47413ca
Resolve circular dependency between traits and ir modules
compor Jun 5, 2023
577c195
Make test condition for null objects explicit
compor May 6, 2023
2590e57
Simplify if condition
compor May 6, 2023
a8aea98
Fix expected message of exception in test
compor May 6, 2023
6a5e84d
Narrow exception types
compor May 6, 2023
4a27aeb
Fix incorrect docstring in test
compor May 6, 2023
beeddc2
Track TODO with github issue
compor Jun 7, 2023
41cc21f
Add verification at the trait
compor Jun 7, 2023
f3ce3eb
Add test for IsTerminator trait verification
compor Jun 7, 2023
7b2dcf6
Add test case
compor Jun 7, 2023
efdd987
Rename test case and clarify its docstring
compor Jun 7, 2023
6822d89
Fix incorrect test docstring
compor Jun 7, 2023
9a3f8d0
Appease pyright
compor Jun 7, 2023
795859f
Move condition for IsTerminator trait so it applies to ops without su…
compor Jun 9, 2023
41f9b1b
Add test case and clarify their docstrings
compor Jun 9, 2023
30d81c9
Fix test case
compor Jun 9, 2023
e476179
Add IsTerminator trait to required ops in cf, scf and func dialects
compor Jun 9, 2023
24686dd
Add test case
compor Jun 9, 2023
b4cedd5
Add terminator operation to test dialect
compor Jun 9, 2023
12d943c
Use test.termop terminator in graph region test
compor Jun 9, 2023
6c22e3d
Check for successors when operation does not belong to block or region
compor Jun 9, 2023
c4d9933
Add test cases checking successors for operations without parent bloc…
compor Jun 9, 2023
2bcbf72
Fix operation builder tests
compor Jun 9, 2023
13cf98c
Check block successors exist only in the last block operation regardl…
compor Jun 9, 2023
b669515
Fix test case
compor Jun 9, 2023
8066cbc
Add docstrings to changed tests
compor Jun 9, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 10 additions & 10 deletions tests/filecheck/parser-printer/graph_region.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,39 @@

builtin.module {
%0 = "test.op"(%1) : (i32) -> i32
%1 = "test.op"(%0) : (i32) -> i32
%1 = "test.termop"(%0) : (i32) -> i32
}

// CHECK: %0 = "test.op"(%1) : (i32) -> i32
// CHECK-NEXT: %1 = "test.op"(%0) : (i32) -> i32
// CHECK-NEXT: %1 = "test.termop"(%0) : (i32) -> i32

// -----

// A self-cycle

builtin.module {
%0 = "test.op"(%0) : (i32) -> i32
%0 = "test.termop"(%0) : (i32) -> i32
}

// CHECK: %0 = "test.op"(%0) : (i32) -> i32
// CHECK: %0 = "test.termop"(%0) : (i32) -> i32

// -----

// A forward value defined by a block argument

builtin.module {
"test.op"() ({
"test.op"(%0) : (i32) -> ()
"test.termop"(%0) : (i32) -> ()
^bb0(%0: i32):
"test.op"() : () -> ()
"test.termop"() : () -> ()
}) : () -> ()
}

// CHECK: builtin.module {
// CHECK-NEXT: "test.op"() ({
// CHECK-NEXT: "test.op"(%0) : (i32) -> ()
// CHECK-NEXT: "test.termop"(%0) : (i32) -> ()
// CHECK-NEXT: ^0(%0 : i32):
// CHECK-NEXT: "test.op"() : () -> ()
// CHECK-NEXT: "test.termop"() : () -> ()
// CHECK-NEXT: }) : () -> ()
// CHECK-NEXT: }

Expand All @@ -46,7 +46,7 @@ builtin.module {
// A graph region that refers to a value that is not defined in the module.

builtin.module {
%0 = "test.op"(%1) : (i32) -> i32
%0 = "test.termop"(%1) : (i32) -> i32
}

// CHECK: value %1 was used but not defined
Expand All @@ -57,7 +57,7 @@ builtin.module {

builtin.module {
"test.op"(%1#3) : (i32) -> ()
%1:3 = "test.op"() : () -> (i32, i32, i32)
%1:3 = "test.termop"() : () -> (i32, i32, i32)
}

// CHECK: SSA value %1 is referenced with an index larger than its size
60 changes: 57 additions & 3 deletions tests/test_builtin_traits.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
import pytest

from xdsl.dialects.builtin import ModuleOp
from xdsl.irdl import IRDLOperation, irdl_op_definition
from xdsl.ir import Region
from xdsl.traits import HasParent
from xdsl.irdl import IRDLOperation, OptSuccessor, irdl_op_definition
from xdsl.ir import Region, Block
from xdsl.traits import HasParent, IsTerminator
from xdsl.utils.exceptions import VerifyException
from xdsl.dialects.test import TestOp


@irdl_op_definition
Expand Down Expand Up @@ -100,3 +101,56 @@ def test_has_parent_verify():

op = Parent2Op(regions=[[HasMultipleParentOp()]])
op.verify()


@irdl_op_definition
class IsTerminatorOp(IRDLOperation):
"""
An operation that provides the IsTerminator trait.
"""

name = "test.is_terminator"

successor: OptSuccessor

traits = frozenset([IsTerminator()])


compor marked this conversation as resolved.
Show resolved Hide resolved
def test_is_terminator_with_successors_verify():
"""
Test that an operation with an IsTerminator trait may have successor blocks.
"""
block0 = Block([])
block1 = Block([IsTerminatorOp.create(successors=[block0])])
region0 = Region([block0, block1])
op0 = TestOp.create(regions=[region0])

op0.verify()


def test_is_terminator_without_successors_verify():
"""
Test that an operation with an IsTerminator trait may not have successor
blocks.
"""
block0 = Block([])
block1 = Block([IsTerminatorOp.create()])
region0 = Region([block0, block1])
op0 = TestOp.create(regions=[region0])

op0.verify()


def test_is_terminator_fails_if_not_last_operation_parent_block():
"""
Test that an operation with an IsTerminator trait fails if it is not the
last operation in its parent block.
"""
block0 = Block([IsTerminatorOp.create(), TestOp.create()])
region0 = Region([block0])
op0 = TestOp.create(regions=[region0])

with pytest.raises(
VerifyException, match="must be the last operation in the parent block"
):
op0.verify()
90 changes: 90 additions & 0 deletions tests/test_ir.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from xdsl.parser import Parser
from xdsl.irdl import IRDLOperation, VarRegion, irdl_op_definition, Operand
from xdsl.utils.test_value import TestSSAValue
from xdsl.utils.exceptions import VerifyException


def test_ops_accessor():
Expand Down Expand Up @@ -162,6 +163,95 @@ def test_region_clone_into_circular_blocks():
assert region.is_structurally_equivalent(region2)


def test_op_with_successors_not_in_block():
block0 = Block()
op0 = TestOp.create(successors=[block0])

with pytest.raises(
VerifyException,
match="Operation with block successors does not belong to a block or a region",
):
op0.verify()


def test_op_with_successors_not_in_region():
block0 = Block()
op0 = TestOp.create(successors=[block0])
_ = Block([op0])

with pytest.raises(
VerifyException,
match="Operation with block successors does not belong to a block or a region",
):
op0.verify()


def test_non_empty_block_with_single_block_parent_region_can_have_terminator():
"""
Tests that an non-empty block belonging to a single-block region with parent
operation can have a single terminator operation without the IsTerminator
trait.
"""
block1 = Block([TestOp.create()])
region0 = Region([block1])
op0 = TestOp.create(regions=[region0])

op0.verify()


def test_non_empty_block_with_parent_region_requires_terminator_with_successors():
"""
Tests that an non-empty block belonging to a multi-block region with parent
operation requires terminator operation.
math-fehr marked this conversation as resolved.
Show resolved Hide resolved
The terminator operation may have successors.
"""
block0 = Block()
block1 = Block([TestOp.create(successors=[block0])])
region0 = Region([block0, block1])
op0 = TestOp.create(regions=[region0])

with pytest.raises(
VerifyException,
match="Operation terminates block but is not a terminator",
):
op0.verify()


def test_non_empty_block_with_parent_region_requires_terminator_without_successors():
"""
Tests that an non-empty block belonging to a multi-block region with parent
operation requires terminator operation.
The terminator operation may not have successors.
"""
block0 = Block()
block1 = Block([TestOp.create()])
region0 = Region([block0, block1])
op0 = TestOp.create(regions=[region0])

with pytest.raises(
VerifyException,
match="Operation terminates block but is not a terminator",
):
op0.verify()


def test_non_empty_block_with_parent_region_has_successors_but_not_last_block_op():
"""
Tests that an non-empty block belonging to a multi-block region with parent
operation requires terminator operation.
"""
block0 = Block()
block1 = Block([TestOp.create(successors=[block0]), TestOp.create()])
region0 = Region([block0, block1])
op0 = TestOp.create(regions=[region0])

with pytest.raises(
VerifyException,
match="Operation with block successors must terminate its parent block",
):
op0.verify()


##################### Testing is_structurally_equal #####################

program_region = """
Expand Down
32 changes: 26 additions & 6 deletions tests/test_operation_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -499,8 +499,12 @@ class SuccessorOp(IRDLOperation):


def test_successor_op_successor():
block = Block()
op = SuccessorOp.build(successors=[block])
block0 = Block()
op = SuccessorOp.build(successors=[block0])

block1 = Block([op])
_ = Region([block1])
compor marked this conversation as resolved.
Show resolved Hide resolved

op.verify()
assert len(op.successors) == 1

Expand All @@ -513,9 +517,13 @@ class OptSuccessorOp(IRDLOperation):


def test_opt_successor_builder():
block = Block()
op1 = OptSuccessorOp.build(successors=[block])
block0 = Block()
op1 = OptSuccessorOp.build(successors=[block0])
op2 = OptSuccessorOp.build(successors=[None])

block1 = Block([op2, op1])
_ = Region([block1])

op1.verify()
op2.verify()

Expand All @@ -528,8 +536,12 @@ class VarSuccessorOp(IRDLOperation):


def test_var_successor_builder():
block = Block()
op = VarSuccessorOp.build(successors=[[block, block, block]])
block0 = Block()
op = VarSuccessorOp.build(successors=[[block0, block0, block0]])

block1 = Block([op])
_ = Region([block1])

op.verify()
assert len(op.successors) == 3

Expand All @@ -549,6 +561,10 @@ def test_two_var_successor_builder():
block3 = Block()
block4 = Block()
op2 = TwoVarSuccessorOp.build(successors=[[block1, block2], [block3, block4]])

block0 = Block([op2])
_ = Region([block0])

op2.verify()
assert op2.successors == [block1, block2, block3, block4]
assert op2.attributes[
Expand All @@ -562,6 +578,10 @@ def test_two_var_successor_builder2():
block3 = Block()
block4 = Block()
op2 = TwoVarSuccessorOp.build(successors=[[block1], [block2, block3, block4]])

block0 = Block([op2])
_ = Region([block0])

op2.verify()
assert op2.successors == [block1, block2, block3, block4]
assert op2.attributes[
Expand Down
5 changes: 5 additions & 0 deletions xdsl/dialects/cf.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from xdsl.dialects.builtin import IntegerType, StringAttr
from xdsl.ir import SSAValue, Operation, Block, Dialect
from xdsl.traits import IsTerminator
from xdsl.irdl import (
OpAttr,
irdl_op_definition,
Expand Down Expand Up @@ -37,6 +38,8 @@ class Branch(IRDLOperation):
arguments: Annotated[VarOperand, AnyAttr()]
successor: Successor

traits = frozenset([IsTerminator()])

@staticmethod
def get(dest: Block, *ops: Union[Operation, SSAValue]) -> Branch:
return Branch.build(operands=[[op for op in ops]], successors=[dest])
Expand All @@ -55,6 +58,8 @@ class ConditionalBranch(IRDLOperation):
then_block: Successor
else_block: Successor

traits = frozenset([IsTerminator()])

@staticmethod
def get(
cond: Union[Operation, SSAValue],
Expand Down
4 changes: 2 additions & 2 deletions xdsl/dialects/func.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
)
from xdsl.parser import Parser
from xdsl.printer import Printer
from xdsl.traits import HasParent
from xdsl.traits import HasParent, IsTerminator
from xdsl.utils.exceptions import VerifyException
from xdsl.utils.hints import isa

Expand Down Expand Up @@ -318,7 +318,7 @@ class Return(IRDLOperation):
name = "func.return"
arguments: Annotated[VarOperand, AnyAttr()]

traits = frozenset([HasParent(FuncOp)])
traits = frozenset([HasParent(FuncOp), IsTerminator()])

def verify_(self) -> None:
func_op = self.parent_op()
Expand Down
3 changes: 3 additions & 0 deletions xdsl/dialects/scf.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from xdsl.dialects.builtin import IndexType, IntegerType
from xdsl.ir import Attribute, Block, Dialect, Operation, Region, SSAValue
from xdsl.traits import IsTerminator
from xdsl.irdl import (
AnyAttr,
AttrSizedOperandSegments,
Expand Down Expand Up @@ -49,6 +50,8 @@ class Yield(IRDLOperation):
name = "scf.yield"
arguments: Annotated[VarOperand, AnyAttr()]

traits = frozenset([IsTerminator()])

@staticmethod
def get(*operands: SSAValue | Operation) -> Yield:
return Yield.create(operands=[SSAValue.get(operand) for operand in operands])
Expand Down