core: Make PatternRewriter a Builder #3540

math-fehr · 2024-11-29T00:48:52Z

Stacked PRs:

core: Make PatternRewriter a Builder

This allows us to now use the ImplicitBuilder on PatternRewriter,
or to use insert using an InsertPoint.

This allows us to now use the ImplicitBuilder on PatternRewriter, or to use `insert` using an InsertPoint. stack-info: PR: #3540, branch: math-fehr/stack/4

codecov · 2024-11-29T00:55:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.36%. Comparing base (7ce487c) to head (a8b1c12).

Additional details and impacted files

@@                Coverage Diff                 @@
##           math-fehr/stack/3    #3540   +/-   ##
==================================================
  Coverage              90.36%   90.36%           
==================================================
  Files                    466      466           
  Lines                  58517    58544   +27     
  Branches                5584     5588    +4     
==================================================
+ Hits                   52878    52905   +27     
  Misses                  4210     4210           
  Partials                1429     1429

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

superlopuh

Fantastico

alexarice

I left some comments but they may be more due to my ignorance than actual problems with the change.

alexarice · 2024-12-02T10:14:01Z

xdsl/pattern_rewriter.py

@@ -91,6 +91,11 @@ class PatternRewriter(PatternRewriterListener):
    has_done_action: bool = field(default=False, init=False)
    """Has the rewriter done any action during the current match."""

+    def __init__(self, current_operation: Operation):


You could possibly do this with a __post_init__

The self.current_operation = current_operation seems like strictly __init__ code to me

I cannot actually do a __post_init__, because if I would use the generated __init__, this would require the arguments for PatternRewriterListener and Builder. But the Builder arguments gets constructed in the __init__ I wrote here.

alexarice · 2024-12-02T10:14:29Z

xdsl/pattern_rewriter.py

@@ -91,6 +91,11 @@ class PatternRewriter(PatternRewriterListener):
    has_done_action: bool = field(default=False, init=False)


Suggested change

has_done_action: bool = field(default=False, init=False)

has_done_action: bool

I think field is dead code if you add an init?

Nope, the default still get added. I removed the init=False though, as we don't have an __init__ here.

alexarice · 2024-12-02T10:16:24Z

xdsl/pattern_rewriter.py

@@ -726,7 +731,11 @@ def _handle_operation_removal(self, op: Operation) -> None:
        """Handle removal of an operation."""
        if self.apply_recursively:
            self._add_operands_to_worklist(op.operands)
-        self._worklist.remove(op)
+        if op.regions:


Is this a separate change to making PatternRewriter a Builder?

I will rebase this branch on a new one, this was indeed a bug before that can be refactored.

alexarice · 2024-12-02T10:17:12Z

xdsl/pattern_rewriter.py

+        else:
+            self._worklist.remove(op)


Suggested change

else:

self._worklist.remove(op)

self._worklist.remove(op)

Surely the operation itself still needs to be removed when it has regions?

op.walk() will walk the current operation as well.

alexarice · 2024-12-02T10:18:38Z

xdsl/rewriter.py

+        if (block := op.parent) is not None:
+            block.erase_op(op, safe_erase=safe_erase)
+        else:
+            op.erase(safe_erase=safe_erase)


Why is it now ok to erase a operation with no parents?

I'll move this to a separate PR as well, the idea is that sometimes we detach an operation and then destroy it.

alexarice · 2024-12-02T10:20:13Z

xdsl/transforms/convert_scf_to_openmp.py

@@ -43,7 +44,8 @@ def match_and_rewrite(self, loop: scf.ParallelOp, rewriter: PatternRewriter, /):
            regions=[Region(Block())],
            operands=[[], [], [], [], [], []],
        )
-        with ImplicitBuilder(parallel.region):
+        rewriter.insertion_point = InsertPoint.at_end(parallel.region.block)
+        with ImplicitBuilder(rewriter):


I don't think I'm a fan of this API. Does the user need to reset the insertion point of the rewriter after doing this?

I agree the API is not optimal.
Currently, what we need to use ImplicitBuilder is:

set the insertion point on the rewriter

Create the ImplicitBuilder using the rewriter

We don't need to reset it after though.

What would you think of ImplicitBuilder(rewriter, at=InsertPoint.at_end(...)), which would set the insertion point just for that region of code?

xdsl/backend/riscv/riscv_scf_to_asm.py

superlopuh · 2024-12-04T10:52:06Z

xdsl/rewriter.py

+        if (block := op.parent) is not None:
+            block.erase_op(op, safe_erase=safe_erase)
+        else:
+            op.erase(safe_erase=safe_erase)


TBH this feels like it could be its own PR, with an accompanying test

Stacked PRs: * #3540 * #3539 * #3538 * __->__#3537 --- --- --- ### transforms: use Rewriter instead of PatternRewriter in mlir-opt `PatternRewriter` should only be used for rewrite patterns.

…il (#3538) Stacked PRs: * #3540 * #3539 * __->__#3538 * #3537 --- --- --- ### transforms: use rewriter and listener in convert-stencil-to-csl-stencil The pass was not propagating the listener from the PatternRewriter, and thus some operations were modified without notifying the rewrite worklist.

…project#3537) Stacked PRs: * xdslproject#3540 * xdslproject#3539 * xdslproject#3538 * __->__#3537 --- --- --- ### transforms: use Rewriter instead of PatternRewriter in mlir-opt `PatternRewriter` should only be used for rewrite patterns.

…il (xdslproject#3538) Stacked PRs: * xdslproject#3540 * xdslproject#3539 * __->__#3538 * xdslproject#3537 --- --- --- ### transforms: use rewriter and listener in convert-stencil-to-csl-stencil The pass was not propagating the listener from the PatternRewriter, and thus some operations were modified without notifying the rewrite worklist.

core: Make PatternRewriter a Builder

a8b1c12

This allows us to now use the ImplicitBuilder on PatternRewriter, or to use `insert` using an InsertPoint. stack-info: PR: #3540, branch: math-fehr/stack/4

math-fehr force-pushed the math-fehr/stack/4 branch from 56bc13b to a8b1c12 Compare November 29, 2024 00:49

math-fehr force-pushed the math-fehr/stack/3 branch from 902ccce to 7ce487c Compare November 29, 2024 00:49

This was referenced Nov 29, 2024

transforms: use Rewriter instead of PatternRewriter in mlir-opt #3537

Merged

transforms: use rewriter and listener in convert-stencil-to-csl-stencil #3538

Merged

transforms: Allow to pass a pattern rewriter in CSE #3539

Open

math-fehr requested review from alexarice and superlopuh November 29, 2024 00:51

math-fehr self-assigned this Nov 29, 2024

math-fehr added the transformations Changes or adds a transformatio label Nov 29, 2024

superlopuh approved these changes Nov 29, 2024

View reviewed changes

alexarice reviewed Dec 2, 2024

View reviewed changes

compor approved these changes Dec 4, 2024

View reviewed changes

xdsl/backend/riscv/riscv_scf_to_asm.py Show resolved Hide resolved

superlopuh reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Make PatternRewriter a Builder #3540

core: Make PatternRewriter a Builder #3540

math-fehr commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

superlopuh left a comment

alexarice left a comment

alexarice Dec 2, 2024

superlopuh Dec 4, 2024

math-fehr Dec 5, 2024

alexarice Dec 2, 2024

math-fehr Dec 5, 2024

alexarice Dec 2, 2024

math-fehr Dec 5, 2024

alexarice Dec 2, 2024

math-fehr Dec 5, 2024

alexarice Dec 2, 2024

math-fehr Dec 5, 2024

alexarice Dec 2, 2024

math-fehr Dec 5, 2024

superlopuh Dec 4, 2024

		@@ -91,6 +91,11 @@ class PatternRewriter(PatternRewriterListener):
		has_done_action: bool = field(default=False, init=False)

	has_done_action: bool = field(default=False, init=False)
	has_done_action: bool

core: Make PatternRewriter a Builder #3540

Are you sure you want to change the base?

core: Make PatternRewriter a Builder #3540

Conversation

math-fehr commented Nov 29, 2024 • edited Loading