feat(x64): fuse *_overflow + brif to jo/jb#13919
Conversation
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| } | ||
| } | ||
|
|
||
| /// Shared fuse-safety checks for matching an overflow-flag value. |
There was a problem hiding this comment.
Unfortunately I don't think we'll want to take this as-is: it's too complex, in an area where we've already had some really subtle and difficult-to-reason-about bugs.
This set of conditions crosses several abstraction levels: it reasons about flag materialization, and especially about "another use of the same value", and then does code-motion if not. That kind of just-so optimization is both precarious (it's distributed invariant maintenance: the main lowering algorithm is responsible for putting defs before uses, but we're overriding that logic on the result of the overflow-checking op because we think it shouldn't matter to do it late in some blocks) and brittle (instruction order can suddenly influence things in unexpected and spooky-action-at-a-distance ways).
For that reason I think this "absorption" is an antipattern, a patch on top of the design that does not fit well with it, and we should instead try to use mechanisms we already have and/or come up with a more general framework that is less brittle and can be more fully integrated into the lowering algorithm.
Would you be willing to attend the next Cranelift meeting? I'd be happy to discuss further and we can try to come up with better approaches here...
There was a problem hiding this comment.
Yes, I would like to join the next Cranelift meeting. How do I get an invite? :)
|
I think the correct way to do this is to formalize a classic, sliding-window-style peephole pass for vcode with a new set of ISLE rules, rather than attempting to, but only half-succeeding to, force it into our existing lowering and rules. This new peephole pass could reuse our existing value use counts from lowering to determine whether the condition needs to be materialized because it is used in other places as well or not. And, fwiw, that issue talks a lot about automata and such, but we could certainly start with a very naive matching strategy and then improve it in tree. Also cc #4124 |
3 participants
Was discussed shortly (#cranelift > Overflow flag is not used for branches?)
Why?
At the moment Cranelift to check for overflow generates something like
add+seto+testand then jumps on the result oftest. While this works, it is not optimal lowering. Better lowering isadd+jowhich removes ~2uops and reduces register pressure as we do not need to store overflow value if it can be fused.