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

cranelift/x64: Narrow test immediate operands #8421

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jameysharp
Copy link
Contributor

The x86 test instruction does a bitwise-and operation, setting flags, but otherwise discarding the result. And when one operand is an immediate constant, any zero bits in the constant can't have an effect on the result. So we can emit shorter versions of this instruction by truncating the most significant zero bits and using the narrowest possible immediate form for the provided constant.

I think this has merge conflicts with #8408 so I'll rebase it next week.

@jameysharp jameysharp requested a review from a team as a code owner April 20, 2024 00:51
@jameysharp jameysharp requested review from abrown and removed request for a team April 20, 2024 00:51
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Apr 20, 2024
The x86 `test` instruction does a bitwise-and operation, setting flags,
but otherwise discarding the result. And when one operand is an
immediate constant, any zero bits in the constant can't have an effect
on the result. So we can emit shorter versions of this instruction by
truncating the most significant zero bits and using the narrowest
possible immediate form for the provided constant.
@jameysharp
Copy link
Contributor Author

It's not clear to me whether I made the right choice to make this change in the code for emitting into the machbuffer, rather than doing it earlier when building vcode.

It's a little weird that the machbuffer disassembly doesn't match the vcode disassembly for an instruction as simple as test.

On the other hand this means that Winch gets this optimization too, and I think it's simpler to implement this in Rust than in ISLE right now.

So I'd be interested in other people's opinions on this: @cfallin @alexcrichton @elliottt?

@alexcrichton
Copy link
Member

Personally I think I'd prefer to do this in ISLE, but I don't feel too strongly really. Given that this is already done here it seems reasonable to land to me

@cfallin
Copy link
Member

cfallin commented Apr 22, 2024

One reason to try to lift optimizations like this into ISLE, and keep the VCode insts as close to 1-to-1 correspondence with machine code as possible, is that it makes verification more tractable: rather than writing a specification for the vcode inst that is "may do this, or that, depending on the constant", we have that logic exposed in ISLE left-hand sides where we can reason about it. For that reason I think I'd prefer for us to do that as well, unless it turns out that it's not really possible or practical at all. (There's definitely room for compromise here and some VCode insts definitely are smarter than they should be! This is more of an aspirational direction for current and future work.)

@jameysharp
Copy link
Contributor Author

Okay, I'll investigate how difficult it is to do this in ISLE and let you all know what I find out, and maybe we can reevaluate then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants