-
Notifications
You must be signed in to change notification settings - Fork 840
Cast optimization improvement: also handle local.tee #6507
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Converts the following:
(some.operation
(ref.cast .. (local.tee $ref ..))
(local.get $ref)
)
into:
(some.operation
(local.tee $temp
(ref.cast .. (local.tee $ref ..))
)
(local.get $temp)
)
This removes close to 10% of the casts in a test program (from 38113
casts down to 36119).
I also tried to improve the optimization that moves more refined casts
earlier, but this does not seem very effective (it only eliminated 9
further casts), so I'm not sure it is worth it.- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -92,22 +92,6 @@ | |||||
| // Note that right now, we only consider RefAs with op RefAsNonNull as a cast. | ||||||
| // RefAs with ExternInternalize and ExternExternalize are not considered casts | ||||||
| // when obtaining fallthroughs, and so are ignored. | ||||||
| // | ||||||
| // TODO: 1. Look past individual basic blocks? This may be worth considering | ||||||
| // given the pattern of a cast appearing in an if condition that is | ||||||
| // then used in an if arm, for example, where simple dominance shows | ||||||
| // the cast can be reused. | ||||||
| // TODO: 2. Look at LocalSet as well and not just Get. That would add some | ||||||
| // overlap with the other passes mentioned above (SimplifyLocals and | ||||||
| // RedundantSetElimination also track sets and can switch a get to use | ||||||
| // a better set's index when that refines the type). But once we do the | ||||||
| // first two TODOs above then we'd be adding some novel things here, | ||||||
| // as we could optimize "backwards" as well (TODO 1) and past basic | ||||||
| // blocks (TODO 2, though RedundantSetElimination does that as well). | ||||||
| // However, we should consider whether improving those other passes | ||||||
| // might make more sense (as it would help more than casts, if we could | ||||||
| // make them operate "backwards" and/or past basic blocks). | ||||||
| // | ||||||
|
|
||||||
| #include "ir/effects.h" | ||||||
| #include "ir/linear-execution.h" | ||||||
|
|
@@ -453,20 +437,32 @@ struct BestCastFinder : public LinearExecutionWalker<BestCastFinder> { | |||||
| void visitRefCast(RefCast* curr) { handleRefinement(curr); } | ||||||
|
|
||||||
| void handleRefinement(Expression* curr) { | ||||||
| auto* teeFallthrough = Properties::getFallthrough( | ||||||
| curr, options, *getModule(), Properties::FallthroughBehavior::NoTeeBrIf); | ||||||
| if (auto* set = teeFallthrough->dynCast<LocalSet>()) { | ||||||
|
||||||
| if (auto* set = teeFallthrough->dynCast<LocalSet>()) { | |
| if (auto* tee = teeFallthrough->dynCast<LocalSet>()) { |
Also the isTee check below is not needed, as a fallthrough LocalSet must be a tee (because only a tee flows out a value that can fall through to some place).
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| auto* fallthrough = Properties::getFallthrough(curr, options, *getModule()); | |
| auto* fallthrough = Properties::getFallthrough(teeFallthrough, options, *getModule()); |
This saves a little work: we looked a little the first time, and the second time we don't need to re-do that work, and just continue from there.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first TODO is still relevant, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it is indeed only partially done (with
connectAdjacentBlocksset to true).