cranelift / PR #1298 Add a DynRex recipe type for x86, de... · git-cranelift

Stream: git-cranelift

Topic: cranelift / PR #1298 Add a DynRex recipe type for x86, de...

GitHub (Dec 18 2019 at 20:45):

sstangl opened PR #1298 from rex2 to master:

[x] This has been discussed in https://github.com/bytecodealliance/cranelift/pull/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.

[x] 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.

[x] This PR contains test cases, if meaningful. (It's covered by the filetests.)

[x] 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:

It is a strict decrease in the number of recipes.

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.

Because the recipe count decreases, there should be a small performance improvement due to faster encoding -- there are fewer options from which to select.

The transformation is fairly mechanical and I believe not difficult to review.

This invalidates the work in https://github.com/bytecodealliance/cranelift/pull/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.

GitHub (Dec 18 2019 at 20:45):

sstangl requested bnjbvr for a review on PR #1298.

GitHub (Dec 18 2019 at 22:50):

iximeow created PR Review Comment:

small quibble: this PR doesn't touch REX prefixes for instructions with memory operands, but this is also a correct check for if the index register in a memory operand needs X. How do you feel about naming this simply is_extended_reg?

GitHub (Dec 18 2019 at 22:50):

iximeow submitted PR Review.

GitHub (Dec 18 2019 at 22:50):

iximeow created PR Review Comment:

This applies to all the size_with_inferred_rex* here - how do you feel about structuring bodies like:
let needs_rex = (bits.rex_w() != 0) || // it'd be handy if `rex_w` could be obtained as a bool, alas
  test_input(0, inst, divert, func, needs_rex_rb) ||
  test_input(1, inst, divert, func, needs_rex_rb);

sizing.base_size + if needs_rex { 1 } else { 0 }
? Where input_test and output_test are additional_size_if but returning only the predicate? Admittedly I'm not a huge fan of additional_size_if because it's a bit eager in turning "we need a prefix" into "it will be so-and-so many bytes" (which would be complicating with @abrown's mention of VEX/EVEX!), so having a generic "is this true for this operand" which we can use to later decide "we do want a REX prefix, and that's one byte", seems nice. I also personally like avoiding the early returns with hardcoded sizes, YMMV!

GitHub (Dec 18 2019 at 22:50):

iximeow submitted PR Review.

GitHub (Dec 18 2019 at 22:50):

iximeow created PR Review Comment:

Same here, but for div's GPR argument. Also thought that the index was off by one, but 2 made me go check the use first :)

GitHub (Dec 18 2019 at 22:50):

iximeow created PR Review Comment:

Can this comment be adjusted to indicate that this refers specifically to the second operand, inreg1, for mulx? Reading this independently had me thinking the operand index was off by one

GitHub (Dec 18 2019 at 22:50):

iximeow submitted PR Review.

GitHub (Dec 18 2019 at 22:51):

iximeow submitted PR Review.

GitHub (Dec 18 2019 at 23:00):

sstangl submitted PR Review.

GitHub (Dec 18 2019 at 23:00):

sstangl created PR Review Comment:

I like it. Will make that change.

GitHub (Dec 18 2019 at 23:22):

sstangl updated PR #1298 from rex2 to master:

[x] This has been discussed in https://github.com/bytecodealliance/cranelift/pull/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.

[x] 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.

[x] This PR contains test cases, if meaningful. (It's covered by the filetests.)

[x] 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:

It is a strict decrease in the number of recipes.

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.

Because the recipe count decreases, there should be a small performance improvement due to faster encoding -- there are fewer options from which to select.

The transformation is fairly mechanical and I believe not difficult to review.

This invalidates the work in https://github.com/bytecodealliance/cranelift/pull/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.

GitHub (Dec 18 2019 at 23:27):

iximeow submitted PR Review.

GitHub (Dec 19 2019 at 09:31):

bnjbvr submitted PR Review.

GitHub (Dec 19 2019 at 09:31):

bnjbvr created PR Review Comment:

nit: here and below, can you expand "in-regs" to "input regs" in comments, please? (and if it's pre-existing, replace it in other places in this file too)

GitHub (Dec 19 2019 at 09:31):

bnjbvr created PR Review Comment:

ditto "two_in_regs" => "inreg_0_1" or "2inregs"? (or conversely, use the longer form for all if you think it's more readable)

GitHub (Dec 19 2019 at 09:31):

bnjbvr submitted PR Review.

GitHub (Dec 19 2019 at 09:31):

bnjbvr created PR Review Comment:

nit: for naming consistency with the other functions, how about size_with_inferred_rex_for_inreg0?

GitHub (Dec 19 2019 at 09:31):

bnjbvr created PR Review Comment:

nit: please expand "out-reg" in comments

GitHub (Dec 19 2019 at 09:31):

bnjbvr created PR Review Comment:

:+1: Can you open an issue for this and write the issue number here please?

GitHub (Dec 19 2019 at 09:31):

bnjbvr created PR Review Comment:

oops, preexisting: these are called Templates since the migration to Rust, not TailRecipes anymore. (a few times below too)

GitHub (Dec 19 2019 at 22:51):

sstangl merged PR #1298.

GitHub (Dec 19 2019 at 22:51):

sstangl updated PR #1298 from rex2 to master:

[x] This has been discussed in https://github.com/bytecodealliance/cranelift/pull/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.

[x] 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.

[x] This PR contains test cases, if meaningful. (It's covered by the filetests.)

[x] 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:

It is a strict decrease in the number of recipes.

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.

Because the recipe count decreases, there should be a small performance improvement due to faster encoding -- there are fewer options from which to select.

The transformation is fairly mechanical and I believe not difficult to review.

This invalidates the work in https://github.com/bytecodealliance/cranelift/pull/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.

GitHub (Dec 21 2019 at 00:22):

bjorn3 submitted PR Review.

GitHub (Dec 21 2019 at 00:22):

bjorn3 created PR Review Comment:

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

GitHub (Dec 21 2019 at 00:22):

bjorn3 created PR Review Comment:

*REZ.X

GitHub (Dec 21 2019 at 00:22):

bjorn3 submitted PR Review.

GitHub (Dec 21 2019 at 00:22):

bjorn3 edited PR Review Comment.

GitHub (Dec 21 2019 at 01:17):

sstangl created PR Review Comment:

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.

GitHub (Dec 21 2019 at 01:17):

sstangl submitted PR Review.

Last updated: Nov 22 2024 at 16:03 UTC