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

Add a DynRex recipe type for x86, decreasing the number of recipes #1298

Merged
merged 3 commits into from Dec 19, 2019

Conversation

@sstangl
Copy link
Collaborator

sstangl commented Dec 18, 2019

  • This has been discussed in #1173. The original approach was to avoid using Templates, but it was extremely complicated. This approach is much better to reduce the number of recipes.
  • This patch adds a third mode for templates: REX inference is requestable at template instantiation time. This reduces the number of recipes by removing rex()/nonrex() redundancy for many instructions. In the common case of i32_i64 and b32_b64 encodings, this inferrable-REX mode is used.
  • This PR contains test cases, if meaningful. (It's covered by the filetests.)
  • A reviewer from the core maintainer team has been assigned for this PR.

I understand that this is work will likely be overridden by what Chris is working on. However, I believe this is still a good idea to land, because:

  1. It is a strict decrease in the number of recipes.
  2. It makes more of the REX-related code explicit, and therefore visible. Removing implicit logic makes the code easier to reason about, and therefore easier to refactor later.
  3. Because the recipe count decreases, there should be a small performance improvement due to faster encoding -- there are fewer options from which to select.
  4. The transformation is fairly mechanical and I believe not difficult to review.

This invalidates the work in #1173. That approach did not work because it tried to avoid using Templates. Removing templates at the same time as making this change resulted in too many moving parts -- it was overly complicated and error-prone.

This patch adds a third mode for templates: REX inference is requestable
at template instantiation time. This reduces the number of recipes
by removing rex()/nonrex() redundancy for many instructions.
@abrown

This comment has been minimized.

Copy link
Collaborator

abrown commented Dec 18, 2019

Do you see a future where this can be extended for VEX/EVEX prefixes?

@sstangl

This comment has been minimized.

Copy link
Collaborator Author

sstangl commented Dec 18, 2019

Yes, in theory. The only reason I would be reticent to do so is that Chris Fallin is working on a prototype for deleting the entire Recipes system, and so I don't know what the future will look like. However if it's not a ton of work and makes the emission-conditions more explicit, that's likely a good idea.

Copy link
Collaborator

iximeow left a comment

In light of plans to just delete all of Recipes (yay), do you plan to extend this for inferring REX on memory-operand encodings, or leave it at this? I have a few comments but overall I'm a big fan of getting encodings more precise with less effort.

cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
@sstangl

This comment has been minimized.

Copy link
Collaborator Author

sstangl commented Dec 18, 2019

Much cleaner now 🙂

In light of plans to just delete all of Recipes (yay), do you plan to extend this for inferring REX on memory-operand encodings, or leave it at this? I have a few comments but overall I'm a big fan of getting encodings more precise with less effort.

I don't have any plans currently. I'll see what Benjamin says.

Copy link
Collaborator

iximeow left a comment

Looking forward to Benjamin's thoughts too!

@bnjbvr
bnjbvr approved these changes Dec 19, 2019
Copy link
Member

bnjbvr left a comment

Nice, thanks! Mostly nits below, so let's (squash and) merge it once these are addressed!

cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift-codegen/src/isa/x86/enc_tables.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/recipes.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/recipes.rs Outdated Show resolved Hide resolved
@sstangl sstangl force-pushed the sstangl:rex2 branch 2 times, most recently from 7c8ffb6 to 6cbb711 Dec 19, 2019
@sstangl sstangl merged commit e0d3172 into bytecodealliance:master Dec 19, 2019
@sstangl sstangl deleted the sstangl:rex2 branch Dec 19, 2019
/// Whether the REX prefix is needed for encoding extended registers (via REX.RXB).
///
/// Normal x86 instructions have only 3 bits for encoding a register.
/// The REX prefix adds REX.R, REX,X, and REX.B bits, interpreted as fourth bits.

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Dec 21, 2019

Contributor

*REX.X

fn put_dynrexop2<CS: CodeSink + ?Sized>(bits: u16, rex: u8, sink: &mut CS) {
debug_assert_eq!(
bits & 0x0f00,
0x0400,

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Dec 21, 2019

Contributor

Doc says 0F (as emitted), yet compared with 04.

This comment has been minimized.

Copy link
@sstangl

sstangl Dec 21, 2019

Author Collaborator

That assertion is copied from 'put_rexopt2(). These assertions should definitely not open-code the bit-patterns, but should be changed to use the new EncodingBits` struct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.