Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Infer REX prefix for SIMD store and vconst instructions #1388

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Feb 12, 2020

  • This has been discussed in issues #1090 and #1379.
  • A short description of what this does, why it is needed: this demonstrates inferring REX prefixes for SIMD store and vconst instructions; if we think this is the right direction, we have some additional to do to infer REX prefixes for the following recipes:

enc_32_64: fstDisp8, fstDisp32, fld, fldDisp8, fldDisp32, fspillSib32, fregspill32, ffillSib32, fregfill32, frmov, furm, fa, fax, f_ib
enc_32_64_maybe_isap: r_ib_unsigned_r, fa_ib, fa, r_ib_unsigned_gpr, fcmp, icscc_fpr
enc_both: frurm, pfcmp, fa, furm

  • This PR contains test cases, if meaningful.
  • A reviewer from the core maintainer team has been assigned for this PR.

@abrown abrown requested a review from iximeow February 12, 2020 23:08
Copy link
Collaborator

@iximeow iximeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the above question, but I'm not sure if I'm misreading, or there's actually a bug, so I'm not explicitly requesting changes.. this looks good overall, but I'm particularly curious if this gets something like mov xmm1, [r11] right. That's the only case (base reg is the only one requiring REX prefix) that I think warrants a closer look.

function %V128() {
block0:
[-,%r10] v3 = iconst.i64 0x2102_0304_f1f2_f3f4 ; bin: 49 ba 21020304f1f2f3f4
[-, %xmm9] v4 = vconst.i32x4 [0 1 2 3] ; bin: 44 0f 10 0d 00000005 PCRelRodata4(23)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@abrown abrown force-pushed the infer-rex-for-simd branch from ae77d49 to 9513fe6 Compare February 13, 2020 18:34
@abrown abrown force-pushed the infer-rex-for-simd branch from 9513fe6 to e98fe1f Compare February 13, 2020 18:48
Copy link
Collaborator

@iximeow iximeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll approve but want an explicitly acknowledgement you're g2g before hitting merge :)

@abrown
Copy link
Collaborator Author

abrown commented Feb 13, 2020

I think we're good-to-go. I'm not really looking forward to doing this same thing for 20+ other recipes so any suggestions are appreciated. The "size_plus_maybe_sib_or_offset_for_inreg_0_plus_rex_prefix_for_inreg0_outreg0" code is not code I would hope to make a career of... is there another way?

@abrown
Copy link
Collaborator Author

abrown commented Feb 13, 2020

...and by "good-to-go" I mean I ran this change with the wasmtime tests (latest on master) and everything passed except some wasm_c_examples for unrelated reasons like make: *** No rule to make target 'wasm-c-api/out/example/reflect-c'

@abrown abrown merged commit 81b7a2e into bytecodealliance:master Feb 19, 2020
@abrown abrown deleted the infer-rex-for-simd branch February 26, 2020 04:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants