Skip to content

Conversation

victor-eds
Copy link
Contributor

@victor-eds victor-eds commented Sep 11, 2023

DO NOT SQUASH AND MERGE THIS PR.

Avoid member functions duplication caused by SingleBlockImplicitTerminator now implying SingleBlock (since 0ac21e6). Use std::enable_if to provide different definitions to push_back and insert.

@victor-eds victor-eds added the sycl-mlir Pull requests or issues for sycl-mlir branch label Sep 11, 2023
@victor-eds victor-eds self-assigned this Sep 11, 2023
@victor-eds victor-eds requested a review from a team as a code owner September 11, 2023 13:05
@victor-eds
Copy link
Contributor Author

Upstream PR: llvm/llvm-project#65959

@victor-eds
Copy link
Contributor Author

Fixes #11078

@victor-eds victor-eds changed the title [mlir] Remove duplicated SingleBlockImplicitTerminator member funions [mlir] Remove duplicated SingleBlockImplicitTerminator member functions Sep 11, 2023
@whitneywhtsang
Copy link
Contributor

Please look into the check-cgeist failures: error: 'gpu.module_end' op must be the last operation in the parent block.

@victor-eds
Copy link
Contributor Author

Just review 9cc35320c1388dea2d7c42560bf0e67bee03d9e6

Copy link
Contributor

@sommerlukas sommerlukas left a comment

Choose a reason for hiding this comment

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

As I told @victor-eds offline, I'm not convinced that std::enable_if_t<!hasSingleBlockImplicitTerminator<OpT> is the right approach here, because it uses the unrelated class SingleBlockImplicitTerminator in the implementation of SingleBlock.

@victor-eds victor-eds marked this pull request as draft September 13, 2023 11:34
@victor-eds victor-eds marked this pull request as ready for review September 15, 2023 14:18
…herited" trait in ODS instead of C++""

This reverts commit ecfd878.

Signed-off-by: Victor Perez <[email protected]>
Change `SingleBlock::{insert,push_back}` to avoid inserting the
argument operation after the block's terminator. This allows removing
`SingleBlockImplicitTerminator`'s functions with the same name.

Define `Block::hasTerminator` checking whether the block has a
terminator operation.

Signed-off-by: Victor Perez <[email protected]>
@victor-eds victor-eds merged commit 67830fe into intel:sycl-mlir Sep 15, 2023
@victor-eds victor-eds deleted the fix-push-back branch September 15, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sycl-mlir Pull requests or issues for sycl-mlir branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants