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

[CIR][Transforms] Simplify redundant bitcasts #591

Merged
merged 5 commits into from
May 8, 2024

Conversation

seven-mile
Copy link
Collaborator

Fix #479 .

There are three available stages to place the simplification in.

  • A straightforward method is to extend fold method for CastOp.
    But CIR does not use CanonicalizerPass, so it does not work.
  • As for somehow equivalent to it, append a pattern to MergeCleanupsPass.
    But now it is mainly for CFG-related simplifications like block merging. I don't know if this is the proper way. Shall we rename it to a broader definition?
  • Add a new pass for this issue.
    This is definitely not very reasonable XD. We won't consider it unless we're really out of options.

This PR includes the second option. What do you think @bcardosolopes ?

@seven-mile seven-mile force-pushed the bitcast-redundancy branch from 7557df9 to 0450e12 Compare May 7, 2024 19:37
@seven-mile
Copy link
Collaborator Author

Oh, it seems applyOpPatternsAndFold will perform DCE on the cast ops that we want to simplify. This broke the CIR/CodeGen/unary.c test. I think the testcase is to blame for unused variables, right?

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Thanks for your first PR, way to go. See comments inline.

@seven-mile seven-mile force-pushed the bitcast-redundancy branch from 4e91da7 to ebafe6f Compare May 8, 2024 03:47
Copy link
Contributor

@victor-eds victor-eds left a comment

Choose a reason for hiding this comment

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

What would be the most appropriate spot for a standalone test for this? Something like:

// CHECK-LABEL: @ptrbitcastfold
//       CHECK:  %[[ARG0:.+]]: !cir.ptr<!i32>
//       CHECK:   return  %[[ARG0]]
func.func @ptrbitcastfold(%arg0: !cir.ptr<!i32>) -> !cir.ptr<!i32> {
  %0 = cir.cast(bitcast, %arg0) : !cir.ptr<!i32>, !cir.ptr<!i32>
  return %0 : !cir.ptr<!i32>
}

@seven-mile
Copy link
Collaborator Author

Thank you all for reviewing and providing so many constructive suggestions! Apart from a few typos, I've tried to address the two NFC commits for testcases. I'm not very familiar with llvm-lit, so please point out any issues if you notice them.

@seven-mile
Copy link
Collaborator Author

I just realized that I probably shouldn't have placed the standalone test under CIR/CodeGen. I'll first merge it into clang/test/CIR/Transforms/merge-cleanups.cir.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@bcardosolopes bcardosolopes merged commit e175549 into llvm:main May 8, 2024
6 checks passed
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
Fix llvm#479 .

There are three available stages to place the simplification in.

* A straightforward method is to extend `fold` method for CastOp.
But CIR does not use CanonicalizerPass, so it does not work.
* As for somehow equivalent to it, append a pattern to
`MergeCleanupsPass`.
But now it is mainly for CFG-related simplifications like block merging.
I don't know if this is the proper way. Shall we rename it to a broader
definition?
* Add a new pass for this issue.
This is definitely not very reasonable XD. We won't consider it unless
we're really out of options.

This PR includes the second option. What do you think @bcardosolopes ?
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
Fix llvm#479 .

There are three available stages to place the simplification in.

* A straightforward method is to extend `fold` method for CastOp.
But CIR does not use CanonicalizerPass, so it does not work.
* As for somehow equivalent to it, append a pattern to
`MergeCleanupsPass`.
But now it is mainly for CFG-related simplifications like block merging.
I don't know if this is the proper way. Shall we rename it to a broader
definition?
* Add a new pass for this issue.
This is definitely not very reasonable XD. We won't consider it unless
we're really out of options.

This PR includes the second option. What do you think @bcardosolopes ?
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
Fix llvm#479 .

There are three available stages to place the simplification in.

* A straightforward method is to extend `fold` method for CastOp.
But CIR does not use CanonicalizerPass, so it does not work.
* As for somehow equivalent to it, append a pattern to
`MergeCleanupsPass`.
But now it is mainly for CFG-related simplifications like block merging.
I don't know if this is the proper way. Shall we rename it to a broader
definition?
* Add a new pass for this issue.
This is definitely not very reasonable XD. We won't consider it unless
we're really out of options.

This PR includes the second option. What do you think @bcardosolopes ?
lanza pushed a commit that referenced this pull request Nov 5, 2024
Fix #479 .

There are three available stages to place the simplification in.

* A straightforward method is to extend `fold` method for CastOp.
But CIR does not use CanonicalizerPass, so it does not work.
* As for somehow equivalent to it, append a pattern to
`MergeCleanupsPass`.
But now it is mainly for CFG-related simplifications like block merging.
I don't know if this is the proper way. Shall we rename it to a broader
definition?
* Add a new pass for this issue.
This is definitely not very reasonable XD. We won't consider it unless
we're really out of options.

This PR includes the second option. What do you think @bcardosolopes ?
lanza pushed a commit that referenced this pull request Mar 18, 2025
Fix #479 .

There are three available stages to place the simplification in.

* A straightforward method is to extend `fold` method for CastOp.
But CIR does not use CanonicalizerPass, so it does not work.
* As for somehow equivalent to it, append a pattern to
`MergeCleanupsPass`.
But now it is mainly for CFG-related simplifications like block merging.
I don't know if this is the proper way. Shall we rename it to a broader
definition?
* Add a new pass for this issue.
This is definitely not very reasonable XD. We won't consider it unless
we're really out of options.

This PR includes the second option. What do you think @bcardosolopes ?
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.

Simplify redundant bitcasts
4 participants