[PATCH 2/5] Support Intel SHA512

[Jiang, Haochen]

> 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