x64: EVEX Encoding for new assembler #11153
Conversation
rahulchaphalkar
requested review from
alexcrichton
and removed request for
a team
github-actions
bot
added
cranelift
|
I'll probably take over this PR soon since @rahulchaphalkar is out for a bit. |
Also, adds some missing `.r()` helpers for consistency.
| inst("vpaddusw", fmt("B", [w(xmm1), r(xmm2), r(xmm_m128)]), vex(L128)._66()._0f().op(0xDD).r(), _64b | compat | avx), | ||
| inst("vphaddw", fmt("B", [w(xmm1), r(xmm2), r(xmm_m128)]), vex(L128)._66()._0f38().op(0x01).r(), _64b | compat | avx), | ||
| inst("vphaddd", fmt("B", [w(xmm1), r(xmm2), r(xmm_m128)]), vex(L128)._66()._0f38().op(0x02).r(), _64b | compat | avx), | ||
| inst("vaddpd", fmt("C", [w(xmm1).k(1).z(), r(xmm2), r(xmm_m128)]), evex(L128)._66()._0f().w1().op(0x58).r(), _64b | compat | avx512vl), |
There was a problem hiding this comment.
This logic should be (AVX512VL AND AVX512F). Additionally, since we need the ability to AND things together, we need to teach the whole expression to mean: (_64b | compat) & (...) (maybe in another PR, since this affects many instructions beyond just this one).
There was a problem hiding this comment.
I think this may actually work out? Not to say our current modeling of things is correct, but functionally with | avx512f here I think it has the net effect of requiring more features. Notably we take all the features the assembler says and flag them all as requirements of each instruction, so by listing more features here it's interpreted as effectively requiring all of them. I don't know if that's an accurate reflection of _64b and compat though as we otherwise basically ignore those features right now.
Although rereading your comment now I suspect you're talking about the interactions of _64b and compat with feature flags, which is exactly the gray area for me that iunno much about, so probably just disregard me
This will require more work to expose these as special, runtime-available flags on the generated instruction.
It's likely they will share logic with the VEX patterns but we can refactor that in the future.
|
@cfallin or @alexcrichton: can one of you review this? I've changed this PR substantially since it was first opened so I think it needs a new reviewer that is not me. |
|
Sure, taking a look! |
Adds evex encoding for new assembler.
Adds
vaddpdevex instr.Following items are not implemented in this PR / left out intentionally -
bcst). Will be implemented in future pr.avx512vlfeature right now, this will be changed to AND ofavx512vlandavx512f, and at a later point toavx10.1,avx10.2etc.vaddpdis not lowered yet@abrown Ready for review. I'll add a few code comments for some discussion soon, apart from above list of omitted items.