Skip to content

Conversation

@AntonLydike
Copy link
Collaborator

Add IsTerminator to scf.reduce op as per upstream

@AntonLydike AntonLydike self-assigned this Sep 2, 2024
@AntonLydike AntonLydike added the dialects Changes on the dialects label Sep 2, 2024
@alexarice
Copy link
Collaborator

I looked at this last week, upstream is significantly different, and in the pinned version of mlir scf.reduce is not a terminator I believe

@JosseVanDelm
Copy link
Collaborator

It indeed became a terminator after the current MLIR version that is pinned:
llvm/llvm-project#75314

I have no clue whether this is a big issue though?
Are there a lot of people who require stuff to be after an scf.reduce po?

@superlopuh
Copy link
Member

This seems like a slippery slope, couldn't we just update MLIR version instead? I would just suggest doing that after the CGO deadline

@alexarice
Copy link
Collaborator

If my understanding is correct, then in the pinned mlir version you can have multiple scf.reduce operations in one scf.parallel, which would not be possible if it were a terminator. The up to date mlir version has been changed so that you can attach multiple regions/arguments. I don't really have any stake in this but it seems like a bad idea to make some strange hybrid scf.reduce op?

@AntonLydike
Copy link
Collaborator Author

Yes, sorry, meant to close this earlier!

@AntonLydike AntonLydike closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dialects Changes on the dialects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants