[PATCH] Support APX CCMP and CTEST

[Cui, Lili]

> On 29.05.2024 08:37, Cui, Lili wrote:
> >> On 23.05.2024 08:12, Cui, Lili wrote:
> >>> With the new macro %NP (no prefix) introduced in this patch, we can
> >> remove %ME of movbe.
> >>
> >> What are you describing here? I can't spot a respective change in the
> >> patch. If this is meant to say "we then could ...", please put such
> >> remarks outside of the commit message area.
> >>
> > It's not part of the commit message, I will submit a separate patch for
> movbe and remove this message in the next version of the CCMP and CTEST
> patch.
> 
> For my own understanding: How would I as the reader have known it's not
> part of the commit message?
> 

Next time I'll add some markers as separators.

> >>> --- a/gas/config/tc-i386.c
> >>> +++ b/gas/config/tc-i386.c
> >>> @@ -416,6 +416,16 @@ struct _i386_insn
> >>>      /* Compressed disp8*N attribute.  */
> >>>      unsigned int memshift;
> >>>
> >>> +    /* SCC = EVEX.[SC3,SC2,SC1,SC0].  */
> >>> +    unsigned int scc;
> >>> +
> >>> +    #define CF 0
> >>> +    #define ZF 1
> >>> +    #define SF 2
> >>> +    #define OF 3
> >>
> >> These names are misleading - they suggest they somehow enumerate
> >> EFLAGS.CF etc. Perhaps OSZC_CF etc?
> >>
> >
> > They belong to oszc_flags, just after the definitions, I will move the
> comments before these definitions to avoid misunderstandings.
> 
> With OSZC_ prefixes added I think the specific placement isn't going to be as
> relevant anymore. Which isn't to say that I object to you moving these closer
> to their corresponding field.
> 
Ok.

> >>> @@ -4290,6 +4309,16 @@ build_apx_evex_prefix (void)
> >>>        || i.tm.opcode_modifier.zu)
> >>>      i.vex.bytes[3] |= 0x10;
> >>>
> >>> +  /* Encode SCC and oszc flags bits.  */  if
> >>> + (i.tm.opcode_modifier.scc)
> >>> +    {
> >>> +      i.vex.bytes[2] &= ~0x78;
> >>
> >> Is there any way these 4 bits may be set before coming here? I hope
> >> there isn't, in which case an assertion (or know()) would seem more
> appropriate.
> >>
> >
> > They are the vvvv bits and the default value should be 1111. We need to
> clear them to 0000.
> 
> Hmm, yes, clearing them is one way of dealing with this. In any event, please
> add a comment here and ...
> 
> >>> +      i.vex.bytes[2] |= (i.oszc_flags << 3);
> >>> +      i.vex.bytes[3] = (i.vex.bytes[3] & 0xf0) | i.scc;
> >>
> >> Same question for these 4 bits, and ...
> >
> > They are V'aaa, V' defaults to 1 and needs to be cleared to 0, but we can add
> a check for aaa.
> 
> ... here to help the reader follow the overlaying of bits (any why some would
> need clearing).
> 
Ok.

> >>> +	  if (op_string[1] != 'f')
> >>> +	    {
> >>> +	      as_bad (_("Unrecognized oszc flags"));
> >>> +	      ignore_rest_of_line ();
> >>> +	      return NULL;
> >>> +	    }
> >>> +	  switch (op_string[0])
> >>> +	    {
> >>> +	    case 'o':
> >>> +	      i.oszc_flags |= (1 << OF);
> >>> +	      break;
> >>> +	    case 's':
> >>> +	      i.oszc_flags |= (1 << SF);
> >>> +	      break;
> >>> +	    case 'z':
> >>> +	      i.oszc_flags |= (1 << ZF);
> >>> +	      break;
> >>> +	    case 'c':
> >>> +	    case 'p':
> >>
> >> I don't think "pf" should be recognized here. As terminology says
> >> here and in the spec, it's OSZC (no P in there).
> >
> >>> +	    case 'a':
> >>> +	      break;
> >>
> >> "af" pretty certainly may not be recognized here, as that would imply
> >> EFLAGS.AF becoming set, not cleared.
> >>
> >
> > I know what you mean, SCC cannot test PF.
> >
> > • If SCC = 0b1010, then SCC evaluates to true regardless of the status flags
> value.
> > • If SCC = 0b1011, then SCC evaluates to false regardless of the status flags
> value.
> > Consequently, the SCC cannot test the parity flag PF.
> 
> All of the code here is solely about OSZC; I don't see why you bring SCC into
> the picture right here.
> 
> > But they are listed in another place, and we should assign PF to EVEX.CF and
> no update for AF.
> >
> > • If SCC evaluates to false on the status flags, then the CMP or TEST
> > is not executed and instead the status flags are updated as follows:
> > – OF = EVEX.OF
> > – SF = EVEX.SF
> > – ZF = EVEX.ZF
> > – CF = EVEX.CF
> > – PF = EVEX.CF
> > – AF = 0
> 
> Yes. But still even in what you write above it's EVEX.CF. There's no EVEX.PF, and
> hence there also shouldn't be {dfv=pf}. (To be honest I would have found it
> clearer if PF, like AF, was simply cleared. But there are likely reasons for it not
> being that way. Sadly such reasoning is never made publicly available ...)
> 

You are right, I should remove PF and AF here. I think it wants to give users a chance to set PF.

> >>
> >>> +	      i.oszc_flags |= (1 << CF);
> >>> +	      break;
> >>
> >> Shouldn't you further reject redundant settings, as in
> >>
> >> 	ccmpe {dfv=cf,cf} ...
> >>
> >> ?
> >
> > How about keeping this compatibility? Like any other pseudo prefix.
> 
> Which "compatibility"? And why the reference to pseudo prefixes when here
> we're dealing with something entirely new, a pseudo suffix?
> Within a single {dfv=...} each flag should be mentioned at most once.
> Anything else is a potential indication of a mistake the programmer made.
> Separately from this we may consider whether to permit more than one
> {dfv=...} for a single insn, with the latter than fully replacing the former's
> effects. Personally I'd recommend against that unless a clear use case could be
> provided, but I wouldn't object to such being done right away.
> 

It's a suffix, but I think we can tolerate multiple repetitions of a flag by just overwriting the previous one with the next one. Like the pseudo-prefix {vex} {vex} . If you insist on giving {dfv=cf,cf}  an error, I'd just put a check before the assignment.

Thanks,
Lili.