Skip to content

Conversation

tommymcm
Copy link
Collaborator

After #1878, we introduced a dependency from the LoopOpInterface on BreakOp.

A similar dependency was resolved for ConditionOp by introducing a ConditionOpInterface.
However, this isn't very scalable since we actually rely on BreakOp methods in LoopOpInterface, where we only relied on the ConditionOp class before.

This PR moves the default implementation of cir::LoopOpInterface::getLoopOpSuccessorRegions into CIRDialect to resolve the dependency.

We may want to think of a more long term solution for handling CIR interfaces.
Should they be a separate build target?
Should they be within CIR dialect?
Should we move the implementation of interfaces into the CIR op themself, like we do for RegionBranchOpInterface? (I prefer this, personally)

…alect.cpp`

Moved implementation to avoid shared library error.
@xlauko
Copy link
Collaborator

xlauko commented Sep 11, 2025

The problem is that LoopOpInterface does not link now with BreakOp?

In this case I would prefer removing the BreakOp dependency and adding LoopBreakingInterface.

@tommymcm
Copy link
Collaborator Author

tommymcm commented Sep 11, 2025

The problem is that LoopOpInterface does not link now with BreakOp?

In this case I would prefer removing the BreakOp dependency and adding LoopBreakingInterface.

If I add some kind of LoopBreakingInterface, then interfaces need to link with CIR traits, since you need to check if the loop is the innermost Breakable parent of the BreakOp.
I think that the interfaces target should only have the interfaces, without default implementations. That way we never have to do these single-purpose interfaces for other operations. It also follows more closely with my understanding of the MLIR interface design.

// Branching from body: go to step (for) or condition.
else if (&op.getBody() == point.getRegionOrNull()) {
// If there are any breaks in the body, also go to exit.
op.getBody().walk([&](cir::BreakOp breakOp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really NFC because you are changing the logic - cir::BreakOp is not there before (nor I did find anything in git history of it being there and getting removed). I understand the test for this will come later after we upstream some passes, and I'm fine with this as is, but the PR should have the accurate info!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ope that's my bad. I thought I had already done the pr to add this handling. Thanks for the catch

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.

3 participants