mbox series

[v3,0/2] x86: insn template CPU specifier processing

Message ID f0214a72-0de4-f353-ae7a-cdc16963a4fe@suse.com
Headers
Series x86: insn template CPU specifier processing |

Message

Jan Beulich Nov. 7, 2023, 1:06 p.m. UTC 
  This continues to be in particular in preparation for APX.

1: Cpu64 handling improvements
2: split insn templates' CPU field

While only noticeable in generated files, this goes on top of "x86:
Intel Core processors do not support CMPXCHG16B" [1]. (Guess how I
noticed that other issue.)

The 1st patch is new in v3, and it being placed first allows what is
now the 2nd patch here to be shrunk some.

Jan

[1] https://sourceware.org/pipermail/binutils/2023-November/130372.html

Comments

Cui, Lili Nov. 8, 2023, 11:43 a.m. UTC | #1 
> Subject: [PATCH v3 0/2] x86: insn template CPU specifier processing
> 
> This continues to be in particular in preparation for APX.
> 
> 1: Cpu64 handling improvements
> 2: split insn templates' CPU field
> 
> While only noticeable in generated files, this goes on top of "x86:
> Intel Core processors do not support CMPXCHG16B" [1]. (Guess how I noticed
> that other issue.)
> 
> The 1st patch is new in v3, and it being placed first allows what is now the 2nd
> patch here to be shrunk some.
> 
> Jan
> 
> [1] https://sourceware.org/pipermail/binutils/2023-November/130372.html

Hi Jan,

I adapted the apx patch based on these 3 patches and they work fine, but I still have two issues. They are all in install_template.

1. We need to clear i.tm.opcode_modifier.evex = 0 or i.tm.opcode_modifier.vex = 0 for all merged insn, I found that output_insn also needs to use i.tm.opcode_modifier.evex. We need to add the cpu id here for each new merge (VEX and EVEX), which looks a bit ugly. Now I just add AMX for it.


     if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2) || maybe_cpu (t, CpuAMX_TILE)
           || maybe_cpu (t, CpuFMA))
          && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL) || maybe_cpu (t, CpuAPX_F)))


2.  The assertion below blocks AMX insns and I need to remove it.
              else
                {
                  //gas_assert (!i.tm.cpu.bitfield.isa);
                  i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
                }

For the first issue, maybe we can optimize it later, except for this, I think these patches are OK.

Regards,
Lili.
Jan Beulich Nov. 8, 2023, 11:49 a.m. UTC | #2 
On 08.11.2023 12:43, Cui, Lili wrote:
>> Subject: [PATCH v3 0/2] x86: insn template CPU specifier processing
>>
>> This continues to be in particular in preparation for APX.
>>
>> 1: Cpu64 handling improvements
>> 2: split insn templates' CPU field
>>
>> While only noticeable in generated files, this goes on top of "x86:
>> Intel Core processors do not support CMPXCHG16B" [1]. (Guess how I noticed
>> that other issue.)
>>
>> The 1st patch is new in v3, and it being placed first allows what is now the 2nd
>> patch here to be shrunk some.
>>
>> Jan
>>
>> [1] https://sourceware.org/pipermail/binutils/2023-November/130372.html
> 
> Hi Jan,
> 
> I adapted the apx patch based on these 3 patches and they work fine, but I still have two issues. They are all in install_template.
> 
> 1. We need to clear i.tm.opcode_modifier.evex = 0 or i.tm.opcode_modifier.vex = 0 for all merged insn, I found that output_insn also needs to use i.tm.opcode_modifier.evex. We need to add the cpu id here for each new merge (VEX and EVEX), which looks a bit ugly. Now I just add AMX for it.
> 
> 
>      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2) || maybe_cpu (t, CpuAMX_TILE)
>            || maybe_cpu (t, CpuFMA))
>           && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL) || maybe_cpu (t, CpuAPX_F)))

I specifically mentioned already that this may want tidying as the set
grows. We can do that up front if you're concerned, or afterwards.

I don't, however, understand how this is connected to you saying "I
found that output_insn also needs to use i.tm.opcode_modifier.evex".

> 2.  The assertion below blocks AMX insns and I need to remove it.
>               else
>                 {
>                   //gas_assert (!i.tm.cpu.bitfield.isa);
>                   i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
>                 }

I had asked about this already in reply to one of the patches, also
pointing out that removing the assertion is problematic. By you not
explaining what the problem is, I still can't figure how this is
getting in the way of AMX. Specifically in that case, what are the
values of both fields upon entry into this block?

Jan
Cui, Lili Nov. 8, 2023, 12:06 p.m. UTC | #3 
> Subject: Re: [PATCH v3 0/2] x86: insn template CPU specifier processing
> 
> On 08.11.2023 12:43, Cui, Lili wrote:
> >> Subject: [PATCH v3 0/2] x86: insn template CPU specifier processing
> >>
> >> This continues to be in particular in preparation for APX.
> >>
> >> 1: Cpu64 handling improvements
> >> 2: split insn templates' CPU field
> >>
> >> While only noticeable in generated files, this goes on top of "x86:
> >> Intel Core processors do not support CMPXCHG16B" [1]. (Guess how I
> >> noticed that other issue.)
> >>
> >> The 1st patch is new in v3, and it being placed first allows what is
> >> now the 2nd patch here to be shrunk some.
> >>
> >> Jan
> >>
> >> [1]
> >> https://sourceware.org/pipermail/binutils/2023-November/130372.html
> >
> > Hi Jan,
> >
> > I adapted the apx patch based on these 3 patches and they work fine, but I
> still have two issues. They are all in install_template.
> >
> > 1. We need to clear i.tm.opcode_modifier.evex = 0 or
> i.tm.opcode_modifier.vex = 0 for all merged insn, I found that output_insn also
> needs to use i.tm.opcode_modifier.evex. We need to add the cpu id here for
> each new merge (VEX and EVEX), which looks a bit ugly. Now I just add AMX
> for it.
> >
> >
> >      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2) || maybe_cpu (t,
> CpuAMX_TILE)
> >            || maybe_cpu (t, CpuFMA))
> >           && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)
> > || maybe_cpu (t, CpuAPX_F)))
> 
> I specifically mentioned already that this may want tidying as the set grows.
> We can do that up front if you're concerned, or afterwards.
> 

May be afterwards.

> I don't, however, understand how this is connected to you saying "I found
> that output_insn also needs to use i.tm.opcode_modifier.evex".
> 
> > 2.  The assertion below blocks AMX insns and I need to remove it.
> >               else
> >                 {
> >                   //gas_assert (!i.tm.cpu.bitfield.isa);
> >                   i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
> >                 }
> 
> I had asked about this already in reply to one of the patches, also pointing out
> that removing the assertion is problematic. By you not explaining what the
> problem is, I still can't figure how this is getting in the way of AMX. Specifically
> in that case, what are the values of both fields upon entry into this block?
> 
Testcase : ldtilecfg  (%rax,%rdx,2)

need_evex_encoding ()  == 0
i.tm.cpu_any.bitfield.cpuavx ==0
i.tm.cpu.bitfield.isa == 110


Lili.
Jan Beulich Nov. 8, 2023, 12:46 p.m. UTC | #4 
On 08.11.2023 13:06, Cui, Lili wrote:
>> Subject: Re: [PATCH v3 0/2] x86: insn template CPU specifier processing
>>
>> On 08.11.2023 12:43, Cui, Lili wrote:
>>> 2.  The assertion below blocks AMX insns and I need to remove it.
>>>               else
>>>                 {
>>>                   //gas_assert (!i.tm.cpu.bitfield.isa);
>>>                   i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
>>>                 }
>>
>> I had asked about this already in reply to one of the patches, also pointing out
>> that removing the assertion is problematic. By you not explaining what the
>> problem is, I still can't figure how this is getting in the way of AMX. Specifically
>> in that case, what are the values of both fields upon entry into this block?
>>
> Testcase : ldtilecfg  (%rax,%rdx,2)
> 
> need_evex_encoding ()  == 0
> i.tm.cpu_any.bitfield.cpuavx ==0
> i.tm.cpu.bitfield.isa == 110

But i.tm.cpu_any.bitfield.isa is what would have been interesting. I
guess I figured the case, though: Both fields are presumably set to
AMX_TILE. I'll rework the code slightly to permit that.

Jan