x86: Process ImmExt without operands

Message ID 20200626172319.GA589940@gmail.com
State New
Headers show
Series
  • x86: Process ImmExt without operands
Related show

Commit Message

Alan Modra via Binutils June 26, 2020, 5:23 p.m.
On Fri, Jun 26, 2020 at 09:26:57AM -0700, H.J. Lu wrote:
> On Fri, Jun 26, 2020 at 9:17 AM Jan Beulich <jbeulich@suse.com> wrote:

> >

> > On 26.06.2020 17:23, H.J. Lu via Binutils wrote:

> > > --- a/opcodes/i386-opc.tbl

> > > +++ b/opcodes/i386-opc.tbl

> > > @@ -84,6 +84,9 @@

> > >  #define Vex128 Vex=VEX128

> > >  #define Vex256 Vex=VEX256

> > >  #define VexLIG Vex=VEXScalar

> > > +#define VexSIB128 SIB=VECSIB128

> > > +#define VecSIB256 SIB=VECSIB256

> > > +#define VecSIB512 SIB=VECSIB512

> >

> > I first thought the typo would be only in the Changelog entry, but

> > here the first one has an x too where the others have a c. Would

> > you mind correcting this, and perhaps also adding a blank line

> > ahead of this new block of #define-s?

> 

> Fixed.

> 

> BTW, this commit:

> 

> commit c423d21a43727fb4d27c10d43e6bbedced0f55d5

> Author: Jan Beulich <jbeulich@suse.com>

> Date:   Thu Jun 25 09:30:09 2020 +0200

> 

>     x86: move ImmExt processing

> 

>     With abuses of ImmExt gone, all templates using it have operands. Move

>     its main invocation into process_operands(), matching its secondary one

>     for the SSE2AVX case.

> 

> makes this AMX entry invalid:

> 

> tilerelease, 0, 0x49, 0xc0, 1, CpuAMX_TILE|Cpu64,

> Vex|VexOpcode=1|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|ImmExt,

> { 0 }

> 

> Any suggestions how to address this?

  
I am checking in this patch.


H.J.
--
To support Intel AMX instructions with 8-bit immediate opcode extension,
but without operands:

tilerelease, 0, 0x49, 0xc0, 1, CpuAMX_TILE|Cpu64, Vex|VexOpcode=1|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|ImmExt, { 0 }

process ImmExt without operands.

	* config/tc-i386.c (md_assemble): Process ImmExt without
	operands.
---
 gas/config/tc-i386.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.26.2

Comments

Jan Beulich June 29, 2020, 7:06 a.m. | #1
On 26.06.2020 19:23, H.J. Lu wrote:
> On Fri, Jun 26, 2020 at 09:26:57AM -0700, H.J. Lu wrote:

>> On Fri, Jun 26, 2020 at 9:17 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>

>>> On 26.06.2020 17:23, H.J. Lu via Binutils wrote:

>>>> --- a/opcodes/i386-opc.tbl

>>>> +++ b/opcodes/i386-opc.tbl

>>>> @@ -84,6 +84,9 @@

>>>>  #define Vex128 Vex=VEX128

>>>>  #define Vex256 Vex=VEX256

>>>>  #define VexLIG Vex=VEXScalar

>>>> +#define VexSIB128 SIB=VECSIB128

>>>> +#define VecSIB256 SIB=VECSIB256

>>>> +#define VecSIB512 SIB=VECSIB512

>>>

>>> I first thought the typo would be only in the Changelog entry, but

>>> here the first one has an x too where the others have a c. Would

>>> you mind correcting this, and perhaps also adding a blank line

>>> ahead of this new block of #define-s?

>>

>> Fixed.

>>

>> BTW, this commit:

>>

>> commit c423d21a43727fb4d27c10d43e6bbedced0f55d5

>> Author: Jan Beulich <jbeulich@suse.com>

>> Date:   Thu Jun 25 09:30:09 2020 +0200

>>

>>     x86: move ImmExt processing

>>

>>     With abuses of ImmExt gone, all templates using it have operands. Move

>>     its main invocation into process_operands(), matching its secondary one

>>     for the SSE2AVX case.

>>

>> makes this AMX entry invalid:

>>

>> tilerelease, 0, 0x49, 0xc0, 1, CpuAMX_TILE|Cpu64,

>> Vex|VexOpcode=1|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|ImmExt,

>> { 0 }

>>

>> Any suggestions how to address this?

>   

> I am checking in this patch.


H.J., may I again ask that you please wait with committing changes
until people have had at least a minimal chance of responding? I
object to this change, since - as per the other reply just sent -
what you try to work around is re-introduction of an abuse of
ImmExt that I've carefully eliminated all prior instances of.

Please revert.

Jan
Alan Modra via Binutils June 29, 2020, 12:09 p.m. | #2
On Mon, Jun 29, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 26.06.2020 19:23, H.J. Lu wrote:

> > On Fri, Jun 26, 2020 at 09:26:57AM -0700, H.J. Lu wrote:

> >> On Fri, Jun 26, 2020 at 9:17 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>

> >>> On 26.06.2020 17:23, H.J. Lu via Binutils wrote:

> >>>> --- a/opcodes/i386-opc.tbl

> >>>> +++ b/opcodes/i386-opc.tbl

> >>>> @@ -84,6 +84,9 @@

> >>>>  #define Vex128 Vex=VEX128

> >>>>  #define Vex256 Vex=VEX256

> >>>>  #define VexLIG Vex=VEXScalar

> >>>> +#define VexSIB128 SIB=VECSIB128

> >>>> +#define VecSIB256 SIB=VECSIB256

> >>>> +#define VecSIB512 SIB=VECSIB512

> >>>

> >>> I first thought the typo would be only in the Changelog entry, but

> >>> here the first one has an x too where the others have a c. Would

> >>> you mind correcting this, and perhaps also adding a blank line

> >>> ahead of this new block of #define-s?

> >>

> >> Fixed.

> >>

> >> BTW, this commit:

> >>

> >> commit c423d21a43727fb4d27c10d43e6bbedced0f55d5

> >> Author: Jan Beulich <jbeulich@suse.com>

> >> Date:   Thu Jun 25 09:30:09 2020 +0200

> >>

> >>     x86: move ImmExt processing

> >>

> >>     With abuses of ImmExt gone, all templates using it have operands. Move

> >>     its main invocation into process_operands(), matching its secondary one

> >>     for the SSE2AVX case.

> >>

> >> makes this AMX entry invalid:

> >>

> >> tilerelease, 0, 0x49, 0xc0, 1, CpuAMX_TILE|Cpu64,

> >> Vex|VexOpcode=1|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|ImmExt,

> >> { 0 }

> >>

> >> Any suggestions how to address this?

> >

> > I am checking in this patch.

>

> H.J., may I again ask that you please wait with committing changes

> until people have had at least a minimal chance of responding? I

> object to this change, since - as per the other reply just sent -

> what you try to work around is re-introduction of an abuse of

> ImmExt that I've carefully eliminated all prior instances of.

>

> Please revert.


Please propose a solution for the problem we are facing.

-- 
H.J.
Alan Modra via Binutils June 29, 2020, 12:25 p.m. | #3
On Mon, Jun 29, 2020 at 5:09 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Mon, Jun 29, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:

> >

> > On 26.06.2020 19:23, H.J. Lu wrote:

> > > On Fri, Jun 26, 2020 at 09:26:57AM -0700, H.J. Lu wrote:

> > >> On Fri, Jun 26, 2020 at 9:17 AM Jan Beulich <jbeulich@suse.com> wrote:

> > >>>

> > >>> On 26.06.2020 17:23, H.J. Lu via Binutils wrote:

> > >>>> --- a/opcodes/i386-opc.tbl

> > >>>> +++ b/opcodes/i386-opc.tbl

> > >>>> @@ -84,6 +84,9 @@

> > >>>>  #define Vex128 Vex=VEX128

> > >>>>  #define Vex256 Vex=VEX256

> > >>>>  #define VexLIG Vex=VEXScalar

> > >>>> +#define VexSIB128 SIB=VECSIB128

> > >>>> +#define VecSIB256 SIB=VECSIB256

> > >>>> +#define VecSIB512 SIB=VECSIB512

> > >>>

> > >>> I first thought the typo would be only in the Changelog entry, but

> > >>> here the first one has an x too where the others have a c. Would

> > >>> you mind correcting this, and perhaps also adding a blank line

> > >>> ahead of this new block of #define-s?

> > >>

> > >> Fixed.

> > >>

> > >> BTW, this commit:

> > >>

> > >> commit c423d21a43727fb4d27c10d43e6bbedced0f55d5

> > >> Author: Jan Beulich <jbeulich@suse.com>

> > >> Date:   Thu Jun 25 09:30:09 2020 +0200

> > >>

> > >>     x86: move ImmExt processing

> > >>

> > >>     With abuses of ImmExt gone, all templates using it have operands. Move

> > >>     its main invocation into process_operands(), matching its secondary one

> > >>     for the SSE2AVX case.

> > >>

> > >> makes this AMX entry invalid:

> > >>

> > >> tilerelease, 0, 0x49, 0xc0, 1, CpuAMX_TILE|Cpu64,

> > >> Vex|VexOpcode=1|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|ImmExt,

> > >> { 0 }

> > >>

> > >> Any suggestions how to address this?

> > >

> > > I am checking in this patch.

> >

> > H.J., may I again ask that you please wait with committing changes

> > until people have had at least a minimal chance of responding? I

> > object to this change, since - as per the other reply just sent -

> > what you try to work around is re-introduction of an abuse of

> > ImmExt that I've carefully eliminated all prior instances of.

> >

> > Please revert.

>

> Please propose a solution for the problem we are facing.

>


How about this?

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index a899d5ec0e8..d34976c4491 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -3679,7 +3679,7 @@ build_vex_prefix (const insn_template *t)
     }
     }

-  switch ((i.tm.base_opcode >> 8) & 0xff)
+  switch ((i.tm.base_opcode >> (i.tm.opcode_length << 3)) & 0xff)
     {
     case 0:
       implied_prefix = 0;
@@ -4888,12 +4888,8 @@ md_assemble (char *line)
       if (!process_operands ())
   return;
     }
-  else
+  else if (!quiet_warnings && i.tm.opcode_modifier.ugh)
     {
-      if (i.tm.opcode_modifier.immext)
-  process_immext ();
-
-      if (!quiet_warnings && i.tm.opcode_modifier.ugh)
       /* UnixWare fsub no args is alias for fsubp, fadd -> faddp, etc.  */
       as_warn (_("translating to `%sp'"), i.tm.name);
     }


-- 
H.J.
Jan Beulich June 29, 2020, 2:40 p.m. | #4
On 29.06.2020 14:25, H.J. Lu wrote:
> How about this?

> 

> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c

> index a899d5ec0e8..d34976c4491 100644

> --- a/gas/config/tc-i386.c

> +++ b/gas/config/tc-i386.c

> @@ -3679,7 +3679,7 @@ build_vex_prefix (const insn_template *t)

>      }

>      }

> 

> -  switch ((i.tm.base_opcode >> 8) & 0xff)

> +  switch ((i.tm.base_opcode >> (i.tm.opcode_length << 3)) & 0xff)

>      {

>      case 0:

>        implied_prefix = 0;

> @@ -4888,12 +4888,8 @@ md_assemble (char *line)

>        if (!process_operands ())

>    return;

>      }

> -  else

> +  else if (!quiet_warnings && i.tm.opcode_modifier.ugh)

>      {

> -      if (i.tm.opcode_modifier.immext)

> -  process_immext ();

> -

> -      if (!quiet_warnings && i.tm.opcode_modifier.ugh)

>        /* UnixWare fsub no args is alias for fsubp, fadd -> faddp, etc.  */

>        as_warn (_("translating to `%sp'"), i.tm.name);

>      }


Looks fine, thanks.

Jan

Patch

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index bae9680515..ae2a2c1a53 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -4873,8 +4873,12 @@  md_assemble (char *line)
       if (!process_operands ())
 	return;
     }
-  else if (!quiet_warnings && i.tm.opcode_modifier.ugh)
+  else
     {
+      if (i.tm.opcode_modifier.immext)
+	process_immext ();
+
+      if (!quiet_warnings && i.tm.opcode_modifier.ugh)
       /* UnixWare fsub no args is alias for fsubp, fadd -> faddp, etc.  */
       as_warn (_("translating to `%sp'"), i.tm.name);
     }