Re: [PATCH 2/5] Support Intel SHA512

[Jan Beulich <jbeulich@suse.com>]

Up-front question on title and naming in the patch: Doc indeed says just
SHA512 (same for SM3 and SM4), but are you (including those who
assigned those names) sure that's going to stay this way by the time
this is merged into the SDM? Considering other ISA names, AVX-SHA512
would seem more consistent to me.

On 13.07.2023 08:33, Haochen Jiang wrote:
> In SHA512 patch, I have considered to eliminate the ModR/M table pass
> for vsha512msg1 and vsha512rnds2 since you just introduced OP_R with
> Uxmm.
> 
> However, xmm_mode in OP_R requires VEX128 or less. But unfortunately,
> for both instructions, they are VEX256. Therefore, I still keep the
> ModR/M table pass in the patch.

I guess I don't (fully) understand. Uxmm and xmm_mode aren't well suited
here anyway. What's wrong with introducing

#define Rxmmq { OP_R, xmmq_mode }

(or Uxmmq) and using it there, rejecting VEX.L==0 just like VEX.L==1 is
rejected for xmm_mode?

> --- a/gas/testsuite/gas/i386/i386.exp
> +++ b/gas/testsuite/gas/i386/i386.exp
> @@ -498,6 +498,8 @@ if [gas_32_check] then {
>      run_list_test "amx-complex-inval"
>      run_dump_test "avx-vnni-int16"
>      run_dump_test "avx-vnni-int16-intel"
> +    run_dump_test "sha512"
> +    run_dump_test "sha512-intel"

Perhaps worth having further tests proving that both assembler and
disassembler correctly deal with (invalid) memory operands / encodings?
(The disassembler part may not need to be a separate test; I think we
already have one which could be extended: disassem.[sd] and its 64-bit
counterpart.)

> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -168,6 +168,8 @@ static const dependency isa_dependencies[] =
>      "LKGS" },
>    { "AVX_VNNI_INT16",
>      "AVX2" },
> +  { "SHA512",
> +    "AVX" },

Like for the earlier patch this wants to move up a little. I also
question that it's AVX that's the baseline feature here. While correct
for SM3, I expect it needs to be AVX2 both here and for SM4, for AVX
offering no real 256-bit integer operations. (Obviously this wants
taking care of in the doc as well.)

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3375,3 +3375,11 @@ vpdpwsud, 0xf3d2, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperand
>  vpdpwsuds, 0xf3d3, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
>  
>  // AVX_VNNI_INT16 instructions end.
> +
> +// SHA512 instructions.
> +
> +vsha512rnds2, 0xf2cb, SHA512, Vex256|Space0F38|Modrm|VexVVVV|VexW0|NoSuf, { RegXMM, RegYMM, RegYMM }
> +vsha512msg1, 0xf2cc, SHA512, Vex256|Space0F38|Modrm|VexW0|NoSuf, { RegXMM, RegYMM }
> +vsha512msg2, 0xf2cd, SHA512, Vex256|Space0F38|Modrm|VexW0|NoSuf, { RegYMM, RegYMM }

Can we please stick to Modrm coming first?

Jan