Stream: git-cranelift

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


view this post on Zulip GitHub (Dec 18 2019 at 20:45):

sstangl opened PR #1298 from rex2 to master:

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 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.

view this post on Zulip GitHub (Dec 18 2019 at 20:45):

sstangl requested bnjbvr for a review on PR #1298.

view this post on Zulip 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?

view this post on Zulip GitHub (Dec 18 2019 at 22:50):

iximeow submitted PR Review.

view this post on Zulip 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!

view this post on Zulip GitHub (Dec 18 2019 at 22:50):

iximeow submitted PR Review.

view this post on Zulip 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 :)

view this post on Zulip 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

view this post on Zulip GitHub (Dec 18 2019 at 22:50):

iximeow submitted PR Review.

view this post on Zulip GitHub (Dec 18 2019 at 22:51):

iximeow submitted PR Review.

view this post on Zulip GitHub (Dec 18 2019 at 23:00):

sstangl submitted PR Review.

view this post on Zulip GitHub (Dec 18 2019 at 23:00):

sstangl created PR Review Comment:

I like it. Will make that change.

view this post on Zulip GitHub (Dec 18 2019 at 23:22):

sstangl updated PR #1298 from rex2 to master:

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 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.

view this post on Zulip GitHub (Dec 18 2019 at 23:27):

iximeow submitted PR Review.

view this post on Zulip GitHub (Dec 19 2019 at 09:31):

bnjbvr submitted PR Review.

view this post on Zulip 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)

view this post on Zulip 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)

view this post on Zulip GitHub (Dec 19 2019 at 09:31):

bnjbvr submitted PR Review.

view this post on Zulip 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?

view this post on Zulip GitHub (Dec 19 2019 at 09:31):

bnjbvr created PR Review Comment:

nit: please expand "out-reg" in comments

view this post on Zulip 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?

view this post on Zulip 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)

view this post on Zulip GitHub (Dec 19 2019 at 22:51):

sstangl merged PR #1298.

view this post on Zulip GitHub (Dec 19 2019 at 22:51):

sstangl updated PR #1298 from rex2 to master:

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 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.

view this post on Zulip GitHub (Dec 21 2019 at 00:22):

bjorn3 submitted PR Review.

view this post on Zulip GitHub (Dec 21 2019 at 00:22):

bjorn3 created PR Review Comment:

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

view this post on Zulip GitHub (Dec 21 2019 at 00:22):

bjorn3 created PR Review Comment:

*REZ.X

view this post on Zulip GitHub (Dec 21 2019 at 00:22):

bjorn3 submitted PR Review.

view this post on Zulip GitHub (Dec 21 2019 at 00:22):

bjorn3 edited PR Review Comment.

view this post on Zulip 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.

view this post on Zulip GitHub (Dec 21 2019 at 01:17):

sstangl submitted PR Review.


Last updated: Dec 23 2024 at 13:07 UTC