RE: [PATCH v3 4/9] Support APX GPR32 with extend evex prefix

["Cui, Lili" <lili.cui@intel.com>]

> >>>>>>>>> @@ -3670,10 +3673,11 @@ install_template (const
> insn_template
> >>>>>>>>> *t)
> >>>>>>>>>
> >>>>>>>>>    /* Dual VEX/EVEX templates need stripping one of the
> >>>>>>>>> possible
> >>>>>> variants.  */
> >>>>>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>>>>>>>> -  {
> >>>>>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> >>>>>>>>> -	   || maybe_cpu (t, CpuFMA))
> >>>>>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t,
> >> CpuAVX512VL)))
> >>>>>>>>> +    {
> >>>>>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) ||
> >> AVX512F(CpuFMA)
> >>>>>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >>>>>>>> APX_F(CpuCMPCCXADD)
> >>>>>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >>>>>>>> APX_F(CpuAVX512DQ)
> >>>>>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
> >>>> APX_F(CpuBMI2))
> >>>>>>>>>  	{
> >>>>>>>>>  	  if (need_evex_encoding ())
> >>>>>>>>
> >>>>>>>> There are several issues here:
> >>>>>>>> - Why did you need to change (to the worse) the original code?
> >>>>>>>> - Why did you not model the addition after that original code?
> >>>>>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
> >>>>>> AVX512
> >>>>>>>> insn can be VEX-encoded?
> >>>>>>>
> >>>>>>>  I don't understand what you mean, we have this combination.
> >>>>>>>
> >>>>>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
> >>>>>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
> >>>>>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
> >>>>>>
> >>>>>> Oh, I'm sorry: I forgot about the mask register insns.
> >>>>>>
> >>>>>>>> - If these new macros are really needed for whatever reason,
> >>>>>>>> they
> >>>>>> shouldn't
> >>>>>>>>   be added to opcodes/i386-opc.h when they're useful only in
> >>>>>>>> the
> >>>>>> assembler.
> >>>>>>>> - Style requires a blank before the opening parenthesis in function
> >>>>>>>>   invocations (which also covers function-like macro invocations).
> >>>>>>>>
> >>>>>>>> I think I asked before: How is it that you get away without
> >>>>>>>> altering cpu_flags_match(), containing related and quite
> >>>>>>>> similar
> >> logic?
> >>>>>>>>
> >>>>>>>
> >>>>>>> For the original logic ( ... || ... ) && ( ... || ...), the
> >>>>>>> content in the first bracket
> >>>>>> and the content in the following brackets can be combined
> >>>>>> arbitrarily. I think it is Inaccurate.
> >>>>>>
> >>>>>> In which way? If there are issues with the existing code, these
> >>>>>> issues want taking care of in separate (prereq) patches. Of
> >>>>>> course there are assumptions made here about the CPU combinations
> >>>>>> that can (and cannot) occur in any of our templates. Similar
> >>>>>> assumptions are imo
> >>>> fine to make in the APX additions.
> >>>>>>
> >>>>>> Note how I used two nested if()s despite that not having been
> >>>>>> necessary at that time. I did so in anticipation that for APX
> >>>>>> you'd want to add another
> >>>>>> (separate) inner if(), rather than altering the one that's there.
> >>>>>
> >>>>> Could we remove the CPU check here? it's a bit ugly and has
> >>>>> limited
> >>>> effectiveness.
> >>>>>
> >>>>>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>>>>     {
> >>>>>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >>>>>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >>>> APX_F(CpuCMPCCXADD)
> >>>>>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >>>> APX_F(CpuAVX512DQ)
> >>>>>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
> >>>>> APX_F(CpuBMI2))
> >>>>
> >>>> I agree on the "a bit ugly" part, but taking what's there right now
> >>>> I don't understand "has limited effectiveness". Of course you can
> >>>> remove any code you want, provided you can prove nothing breaks.
> >>>>
> >>>
> >>> Here is install_template().
> >>> All I can say is that after removing the CPU check, no test cases
> >>> failed. I
> >> know it's hard to convince you to delete this place, or what do you
> >> suggest to do with this? APX requires this, otherwise the test cases will fail.
> >>>
> >>> -      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >>> -         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >> APX_F(CpuCMPCCXADD)
> >>> -         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >> APX_F(CpuAVX512DQ)
> >>> -         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> >>> -       {
> >>
> >> So be it then (assuming you don't delete any pre-existing code
> >> there). As said, I expect this will bite us later.
> >
> > Done.
> 
> I can't connect this with ...
> 
> > +      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> > +          || maybe_cpu (t, CpuFMA))
> > +         && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL))
> > +         || APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) ||
> APX_F(CpuAVX512F)
> > +         || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) ||
> APX_F(CpuBMI)
> > +         || APX_F(CpuBMI2))
> 
> ... this: You said you want to remove all the new checks. And now you say
> "done" with the checks all still there? And even if I misunderstood you, I still
> don't see why you'd modify the existing condition: The adjustments made in
> the body of the if() aren't applicable to APX afaict. Plus there are still the odd
> APX_F() uses; I'm sure I commented on that before. If any adjustments need
> making for APX, you want to add a 2nd inner if() inside the enclosing one.
> 

I want to remove all, including your pre-existing code,  there is an EVEX testcase failure due to not clean i.tm.opcode_modifier.vex = 0;  As you required that don't delete any pre-existing code, so I still need to add my new combination,  

How about this ?


I want to remove all code, including your pre-existing code, VEX test case fails because it wasn't cleaned up i.tm.opcode_modifier.evex = 0; As you asked, don't remove any pre-existing code, so I still need to add my new combinations.

How about this?

  /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
  if (t->opcode_modifier.vex && t->opcode_modifier.evex)
    {
      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
           || maybe_cpu (t, CpuFMA))
          && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
        {
          if (need_evex_encoding ())
            {
              i.tm.opcode_modifier.vex = 0;
              i.tm.cpu.bitfield.cpuavx512f = i.tm.cpu_any.bitfield.cpuavx512f;
              i.tm.cpu.bitfield.cpuavx512vl = i.tm.cpu_any.bitfield.cpuavx512vl;
            }
          else
            {
              i.tm.opcode_modifier.evex = 0;
              if (i.tm.cpu_any.bitfield.cpuavx)
                i.tm.cpu.bitfield.cpuavx = 1;
              else if (!i.tm.cpu.bitfield.isa)
                i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
              else
                gas_assert (i.tm.cpu.bitfield.isa == i.tm.cpu_any.bitfield.isa);
            }
        }

      if (APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F)
          || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) || APX_F(CpuBMI)
          || APX_F(CpuBMI2))
        if (need_evex_encoding ())
          i.tm.opcode_modifier.vex = 0;
        else
          i.tm.opcode_modifier.evex = 0;
    }

Thanks,
Lili.