[2/5] Support Intel SHA512

  Hi Jan,

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.

BRs,
Haochen

gas/ChangeLog:

	* NEWS: Support Intel SHA512.
	* config/tc-i386.c: Add sha512.
	* doc/c-i386.texi: Document .sha512.
	* testsuite/gas/i386/i386.exp: Run SHA512 tests.
	* testsuite/gas/i386/x86-64.exp: Ditto.
	* testsuite/gas/i386/sha512-intel.d: New test.
	* testsuite/gas/i386/sha512-inval.l: Ditto.
	* testsuite/gas/i386/sha512-inval.s: Ditto.
	* testsuite/gas/i386/sha512.d: Ditto.
	* testsuite/gas/i386/sha512.s: Ditto.
	* testsuite/gas/i386/x86-64-sha512-intel.d: Ditto.
	* testsuite/gas/i386/x86-64-sha512-inval.l: Ditto.
	* testsuite/gas/i386/x86-64-sha512-inval.s: Ditto.
	* testsuite/gas/i386/x86-64-sha512.d: Ditto.
	* testsuite/gas/i386/x86-64-sha512.s: Ditto.

opcodes/ChangeLog:

	* i386-dis.c (Uymm): New.
	(MOD_VEX_0F38CB_P_3_W_0_L_1): Ditto.
	(MOD_VEX_0F38CC_P_3_W_0_L_1): Ditto.
	(PREFIX_VEX_0F38CB): Ditto.
	(PREFIX_VEX_0F38CC): Ditto.
	(PREFIX_VEX_0F38CD): Ditto.
	(VEX_LEN_0F38CB_P_3_W_0): Ditto.
	(VEX_LEN_0F38CC_P_3_W_0): Ditto.
	(VEX_LEN_0F38CD_P_3_W_0): Ditto.
	(VEX_W_0F38CB_P_3): Ditto.
	(VEX_W_0F38CC_P_3): Ditto.
	(VEX_W_0F38CD_P_3): Ditto.
	(mod_table): Add MOD_VEX_0F38CB_P_3_W_0_L_1, MOD_VEX_0F38CC_P_3_W_0_L_1,
	(prefix_table): Add PREFIX_VEX_0F38CB, PREFIX_VEX_0F38CC,
	PREFIX_VEX_0F38CD.
	(vex_len_table): Add VEX_LEN_0F38CB_P_3_W_0,
	VEX_LEN_0F38CC_P_3_W_0, VEX_LEN_0F38CD_P_3_W_0.
	(vex_w_table): Add VEX_W_0F38CB_P_3, VEX_W_0F38CC_P_3, VEX_W_0F38CD_P_3.
	* i386-gen.c (isa_dependencies): Add SHA512.
	(cpu_flags): Ditto.
	* i386-init.h: Regenerated.
	* i386-mnem.h: Ditto.
	* i386-opc.h (CpuSHA512): New.
	(i386_cpu_flags): Add cpusha512.
	* i386-opc.tbl: Add SHA512 instructions.
	* i386-tbl.h: Regenerated.
---
 gas/NEWS                                     |    2 +
 gas/config/tc-i386.c                         |    1 +
 gas/doc/c-i386.texi                          |    3 +-
 gas/testsuite/gas/i386/i386.exp              |    2 +
 gas/testsuite/gas/i386/sha512-intel.d        |   16 +
 gas/testsuite/gas/i386/sha512.d              |   16 +
 gas/testsuite/gas/i386/sha512.s              |   13 +
 gas/testsuite/gas/i386/x86-64-sha512-intel.d |   16 +
 gas/testsuite/gas/i386/x86-64-sha512.d       |   16 +
 gas/testsuite/gas/i386/x86-64-sha512.s       |   13 +
 gas/testsuite/gas/i386/x86-64.exp            |    2 +
 opcodes/i386-dis.c                           |   82 +-
 opcodes/i386-gen.c                           |    3 +
 opcodes/i386-init.h                          |  648 +-
 opcodes/i386-mnem.h                          | 3949 ++++----
 opcodes/i386-opc.h                           |    3 +
 opcodes/i386-opc.tbl                         |    8 +
 opcodes/i386-tbl.h                           | 8555 +++++++++---------
 18 files changed, 6806 insertions(+), 6542 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/sha512-intel.d
 create mode 100644 gas/testsuite/gas/i386/sha512.d
 create mode 100644 gas/testsuite/gas/i386/sha512.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-sha512-intel.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-sha512.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-sha512.s
  

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
  

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

SHA512 is not an ISA under AVX set. So AVX-SHA512 is not used.

The actual meaning in SDM/ISE is that we need to check both AVX and SHA512
feature bit to use the instruction.

I could drop the imply in implementation and change to checking both ISA bit
set. But since it will use xmm/ymm register, in current implementation, we
choose to imply AVX for SHA512 for convenience.

Whether it should be AVX/AVX2 will be mentioned below.

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

Since xmm_mode and xmmq_mode does same under VEX.L==1, it could
be used here. I will change to that.

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

I will try to add that in next version.

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

You got a point here.

I will check with the design and HW team since it is actually AVX2 introduces the
256-bit integer operations to see if this is a misuse.

One reason I can think of using AVX only is that SHA512 and SM4 actually do not
need other integer operations to help with. It only needs VMOV, which is introduced
by AVX. So when hardware checking XSTATE, AVX is enough.

Thx,
Haochen
  

On 14.07.2023 05:40, Jiang, Haochen wrote:
>> 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.
> 
> SHA512 is not an ISA under AVX set.

I'm afraid I don't understand. How is it not? It uses YMM registers.
And conceivably there could be EVEX encodings of these (allowing the
full 32 register set to be used), which I'd then call AVX512-SHA512.

It's also not possible to potentially express the same thing in
legacy encodings (unlike e.g. GFNI). Even for SM3, where only 128-
bit operations are used, that's not possible, as the insns have 3
inputs (the destination is r/w).

> So AVX-SHA512 is not used.
> 
> The actual meaning in SDM/ISE is that we need to check both AVX and SHA512
> feature bit to use the instruction.
> 
> I could drop the imply in implementation and change to checking both ISA bit
> set. But since it will use xmm/ymm register, in current implementation, we
> choose to imply AVX for SHA512 for convenience.
> 
> Whether it should be AVX/AVX2 will be mentioned below.
>[...]
>>> --- 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.)
> 
> You got a point here.
> 
> I will check with the design and HW team since it is actually AVX2 introduces the
> 256-bit integer operations to see if this is a misuse.
> 
> One reason I can think of using AVX only is that SHA512 and SM4 actually do not
> need other integer operations to help with. It only needs VMOV, which is introduced
> by AVX. So when hardware checking XSTATE, AVX is enough.

So for a feature check requirement referencing just AVX may be okay. But
there's not going to be any SHA512 without AVX anyway, for there not
being any YMM registers without AVX; you wouldn't be able to fill the
register operands. Hence the extra feature check is redundant (and would
hence better be omitted).

As to implying baseline functionality, using AVX (rather than AVX2) makes
little sense, so even if the feature check remained (note that various
other extensions, including e.g. AVX-VNNI-INT<n>, don't have such a
secondary requirement), I'd still be fairly insistent on having the
base feature named here (and for SM4) be AVX2 (to be in line with other
similar baseline selections).

Jan
  

> I'm afraid I don't understand. How is it not? It uses YMM registers.
> And conceivably there could be EVEX encodings of these (allowing the
> full 32 register set to be used), which I'd then call AVX512-SHA512.
> 
> It's also not possible to potentially express the same thing in
> legacy encodings (unlike e.g. GFNI). Even for SM3, where only 128-
> bit operations are used, that's not possible, as the insns have 3
> inputs (the destination is r/w).

I am actually expressing that to the same thing as GFNI although it does not
has legacy encoding.

Actually, we somehow want to show the evolution from previous SHA. I will
move the entry of them just after the SHA since they are both crypto related
ISAs.

> [...]
> So for a feature check requirement referencing just AVX may be okay. But
> there's not going to be any SHA512 without AVX anyway, for there not
> being any YMM registers without AVX; you wouldn't be able to fill the
> register operands. Hence the extra feature check is redundant (and would
> hence better be omitted).
> 
> As to implying baseline functionality, using AVX (rather than AVX2) makes
> little sense, so even if the feature check remained (note that various
> other extensions, including e.g. AVX-VNNI-INT<n>, don't have such a
> secondary requirement), I'd still be fairly insistent on having the
> base feature named here (and for SM4) be AVX2 (to be in line with other
> similar baseline selections).

I confirmed that AVX in doc here means a state of the whole AVX ISA,
which should include AVX and AVX2. 

I will change the imply of SHA512 and SM4 to AVX2 since it looks much more
reasonable.

Should we also change the imply of SM3 here?

Thx,
Haochen

> 
> Jan
  

On 18.07.2023 09:20, Jiang, Haochen wrote:
>> As to implying baseline functionality, using AVX (rather than AVX2) makes
>> little sense, so even if the feature check remained (note that various
>> other extensions, including e.g. AVX-VNNI-INT<n>, don't have such a
>> secondary requirement), I'd still be fairly insistent on having the
>> base feature named here (and for SM4) be AVX2 (to be in line with other
>> similar baseline selections).
> 
> I confirmed that AVX in doc here means a state of the whole AVX ISA,
> which should include AVX and AVX2. 
> 
> I will change the imply of SHA512 and SM4 to AVX2 since it looks much more
> reasonable.

Thanks.

> Should we also change the imply of SM3 here?

AVX looks sufficient there, so I'd say only if you have a good justification.

Jan