x86: relax redundant REX prefix check

Message ID 5B0E55FD02000078001C6D8F@prv1-mh.provo.novell.com
State New
Headers show
Series
  • x86: relax redundant REX prefix check
Related show

Commit Message

Jan Beulich May 30, 2018, 7:42 a.m.
All REX bits can be specified via individual prefixes. Redundancy should
only be reported on a per-bit basis.

Note that I originally had further checks added to the test case,
checking the effect also on PDEP. I had to strip those, because my patch
to correctly handle those
(https://sourceware.org/ml/binutils/2017-02/msg00280.html) was rejected.
I continue to think that there should not be any new prefix introduced
to handle the VEX case - whether the encoding of an insn requires VEX et
al should not be of immediate interest to the programmer.

gas/
2018-05-30  Jan Beulich  <jbeulich@suse.com>

	* config/tc-i386.c (add_prefix): Check REX bits individually.
	* testsuite/gas/i386/rex.s: Add tests for overriding individual
	REX bits, including when others are already set.
	* testsuite/gas/i386/ilp32/rex.d, testsuite/gas/i386/rex.d:
	Adjust expectations.

Comments

H.J. Lu May 30, 2018, 12:30 p.m. | #1
On Wed, May 30, 2018 at 12:42 AM, Jan Beulich <JBeulich@suse.com> wrote:
> All REX bits can be specified via individual prefixes. Redundancy should

> only be reported on a per-bit basis.

>

> Note that I originally had further checks added to the test case,

> checking the effect also on PDEP. I had to strip those, because my patch

> to correctly handle those

> (https://sourceware.org/ml/binutils/2017-02/msg00280.html) was rejected.

> I continue to think that there should not be any new prefix introduced

> to handle the VEX case - whether the encoding of an insn requires VEX et

> al should not be of immediate interest to the programmer.

>

> gas/

> 2018-05-30  Jan Beulich  <jbeulich@suse.com>

>

>         * config/tc-i386.c (add_prefix): Check REX bits individually.

>         * testsuite/gas/i386/rex.s: Add tests for overriding individual

>         REX bits, including when others are already set.

>         * testsuite/gas/i386/ilp32/rex.d, testsuite/gas/i386/rex.d:

>         Adjust expectations.

>

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

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

> @@ -2328,8 +2328,9 @@ add_prefix (unsigned int prefix)

>        && flag_code == CODE_64BIT)

>      {

>        if ((i.prefix[REX_PREFIX] & prefix & REX_W)

> -         || ((i.prefix[REX_PREFIX] & (REX_R | REX_X | REX_B))

> -             && (prefix & (REX_R | REX_X | REX_B))))

> +         || (i.prefix[REX_PREFIX] & prefix & REX_R)

> +         || (i.prefix[REX_PREFIX] & prefix & REX_X)

> +         || (i.prefix[REX_PREFIX] & prefix & REX_B))

>         ret = PREFIX_EXIST;

>        q = REX_PREFIX;

>      }

> --- a/gas/testsuite/gas/i386/ilp32/rex.d

> +++ b/gas/testsuite/gas/i386/ilp32/rex.d

> @@ -16,6 +16,14 @@ Disassembly of section .text:

>  [       ]*[0-9a-f]+:[   ]+4a 0f ae 04 05 00 00 00 00[   ]+fxsave64[     ]+(0x0)?\(,%r8(,1)?\)

>  [       ]*[0-9a-f]+:[   ]+43 0f ae 04 00[       ]+fxsave[       ]+\(%r8,%r8(,1)?\)

>  [       ]*[0-9a-f]+:[   ]+4b 0f ae 04 00[       ]+fxsave64[     ]+\(%r8,%r8(,1)?\)

> +[       ]*[0-9a-f]+:[   ]+48 03 04 00[  ]+add[  ]+\(%rax,%rax(,1)?\),%rax

> +[       ]*[0-9a-f]+:[   ]+44 03 04 00[  ]+add[  ]+\(%rax,%rax(,1)?\),%r8d

> +[       ]*[0-9a-f]+:[   ]+41 03 04 00[  ]+add[  ]+\(%r8,%rax(,1)?\),%eax

> +[       ]*[0-9a-f]+:[   ]+42 03 04 00[  ]+add[  ]+\(%rax,%r8(,1)?\),%eax

> +[       ]*[0-9a-f]+:[   ]+49 03 04 00[  ]+add[  ]+\(%r8,%rax(,1)?\),%rax

> +[       ]*[0-9a-f]+:[   ]+46 03 04 00[  ]+add[  ]+\(%rax,%r8(,1)?\),%r8d

> +[       ]*[0-9a-f]+:[   ]+45 03 04 00[  ]+add[  ]+\(%r8,%rax(,1)?\),%r8d

> +[       ]*[0-9a-f]+:[   ]+4a 03 04 00[  ]+add[  ]+\(%rax,%r8(,1)?\),%rax

>  [       ]*[0-9a-f]+:[   ]+41\s+rex\.B

>  [       ]*[0-9a-f]+:[   ]+9b dd 30\s+fsave\s+\(%rax\)

>  [       ]*[0-9a-f]+:[   ]+9b 41 dd 30\s+fsave\s+\(%r8\)

> --- a/gas/testsuite/gas/i386/rex.d

> +++ b/gas/testsuite/gas/i386/rex.d

> @@ -15,6 +15,14 @@ Disassembly of section .text:

>  [       ]*[0-9a-f]+:[   ]+4a 0f ae 04 05 00 00 00 00[   ]+fxsave64[     ]+(0x0)?\(,%r8(,1)?\)

>  [       ]*[0-9a-f]+:[   ]+43 0f ae 04 00[       ]+fxsave[       ]+\(%r8,%r8(,1)?\)

>  [       ]*[0-9a-f]+:[   ]+4b 0f ae 04 00[       ]+fxsave64[     ]+\(%r8,%r8(,1)?\)

> +[       ]*[0-9a-f]+:[   ]+48 03 04 00[  ]+add[  ]+\(%rax,%rax(,1)?\),%rax

> +[       ]*[0-9a-f]+:[   ]+44 03 04 00[  ]+add[  ]+\(%rax,%rax(,1)?\),%r8d

> +[       ]*[0-9a-f]+:[   ]+41 03 04 00[  ]+add[  ]+\(%r8,%rax(,1)?\),%eax

> +[       ]*[0-9a-f]+:[   ]+42 03 04 00[  ]+add[  ]+\(%rax,%r8(,1)?\),%eax

> +[       ]*[0-9a-f]+:[   ]+49 03 04 00[  ]+add[  ]+\(%r8,%rax(,1)?\),%rax

> +[       ]*[0-9a-f]+:[   ]+46 03 04 00[  ]+add[  ]+\(%rax,%r8(,1)?\),%r8d

> +[       ]*[0-9a-f]+:[   ]+45 03 04 00[  ]+add[  ]+\(%r8,%rax(,1)?\),%r8d

> +[       ]*[0-9a-f]+:[   ]+4a 03 04 00[  ]+add[  ]+\(%rax,%r8(,1)?\),%rax

>  [       ]*[0-9a-f]+:[   ]+41\s+rex\.B

>  [       ]*[0-9a-f]+:[   ]+9b dd 30\s+fsave\s+\(%rax\)

>  [       ]*[0-9a-f]+:[   ]+9b 41 dd 30\s+fsave\s+\(%r8\)

> --- a/gas/testsuite/gas/i386/rex.s

> +++ b/gas/testsuite/gas/i386/rex.s

> @@ -10,6 +10,16 @@ _start:

>         rex/fxsave (%r8,%r8)

>         rex64/fxsave (%r8,%r8)

>

> +       rex.w add       (%rax,%rax), %eax

> +       rex.r add       (%rax,%rax), %eax

> +       rex.b add       (%rax,%rax), %eax

> +       rex.x add       (%rax,%rax), %eax

> +

> +       rex.w add       (%r8,%rax), %eax

> +       rex.r add       (%rax,%r8), %eax

> +       rex.b add       (%rax,%rax), %r8d


Since R and B bits have been set in this case, shouldn't rex.r/rex.b
be the extra
REX prefix?

> +       rex.x add       (%rax,%rax), %rax

> +

>




-- 
H.J.
Jan Beulich May 30, 2018, 2:22 p.m. | #2
>>> On 30.05.18 at 14:30, <hjl.tools@gmail.com> wrote:

>> --- a/gas/testsuite/gas/i386/rex.s

>> +++ b/gas/testsuite/gas/i386/rex.s

>> @@ -10,6 +10,16 @@ _start:

>>         rex/fxsave (%r8,%r8)

>>         rex64/fxsave (%r8,%r8)

>>

>> +       rex.w add       (%rax,%rax), %eax

>> +       rex.r add       (%rax,%rax), %eax

>> +       rex.b add       (%rax,%rax), %eax

>> +       rex.x add       (%rax,%rax), %eax

>> +

>> +       rex.w add       (%r8,%rax), %eax

>> +       rex.r add       (%rax,%r8), %eax

>> +       rex.b add       (%rax,%rax), %r8d

> 

> Since R and B bits have been set in this case, shouldn't rex.r/rex.b

> be the extra

> REX prefix?


I may not understand your question. In all of the cases prefix and
operands specify non-overlapping sets of REX bits. In this last
quoted line, only REX.R gets set from processing the operands.

Jan
H.J. Lu May 30, 2018, 2:31 p.m. | #3
On Wed, May 30, 2018 at 7:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.05.18 at 14:30, <hjl.tools@gmail.com> wrote:

>>> --- a/gas/testsuite/gas/i386/rex.s

>>> +++ b/gas/testsuite/gas/i386/rex.s

>>> @@ -10,6 +10,16 @@ _start:

>>>         rex/fxsave (%r8,%r8)

>>>         rex64/fxsave (%r8,%r8)

>>>

>>> +       rex.w add       (%rax,%rax), %eax

>>> +       rex.r add       (%rax,%rax), %eax

>>> +       rex.b add       (%rax,%rax), %eax

>>> +       rex.x add       (%rax,%rax), %eax

>>> +

>>> +       rex.w add       (%r8,%rax), %eax

>>> +       rex.r add       (%rax,%r8), %eax

>>> +       rex.b add       (%rax,%rax), %r8d

>>

>> Since R and B bits have been set in this case, shouldn't rex.r/rex.b

>> be the extra

>> REX prefix?

>

> I may not understand your question. In all of the cases prefix and

> operands specify non-overlapping sets of REX bits. In this last

> quoted line, only REX.R gets set from processing the operands.

>


My mistake.  Patch is OK.  Thanks.


-- 
H.J.

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2328,8 +2328,9 @@  add_prefix (unsigned int prefix)
       && flag_code == CODE_64BIT)
     {
       if ((i.prefix[REX_PREFIX] & prefix & REX_W)
-	  || ((i.prefix[REX_PREFIX] & (REX_R | REX_X | REX_B))
-	      && (prefix & (REX_R | REX_X | REX_B))))
+	  || (i.prefix[REX_PREFIX] & prefix & REX_R)
+	  || (i.prefix[REX_PREFIX] & prefix & REX_X)
+	  || (i.prefix[REX_PREFIX] & prefix & REX_B))
 	ret = PREFIX_EXIST;
       q = REX_PREFIX;
     }
--- a/gas/testsuite/gas/i386/ilp32/rex.d
+++ b/gas/testsuite/gas/i386/ilp32/rex.d
@@ -16,6 +16,14 @@  Disassembly of section .text:
 [	 ]*[0-9a-f]+:[	 ]+4a 0f ae 04 05 00 00 00 00[	 ]+fxsave64[	 ]+(0x0)?\(,%r8(,1)?\)
 [	 ]*[0-9a-f]+:[	 ]+43 0f ae 04 00[	 ]+fxsave[	 ]+\(%r8,%r8(,1)?\)
 [	 ]*[0-9a-f]+:[	 ]+4b 0f ae 04 00[	 ]+fxsave64[	 ]+\(%r8,%r8(,1)?\)
+[	 ]*[0-9a-f]+:[	 ]+48 03 04 00[	 ]+add[	 ]+\(%rax,%rax(,1)?\),%rax
+[	 ]*[0-9a-f]+:[	 ]+44 03 04 00[	 ]+add[	 ]+\(%rax,%rax(,1)?\),%r8d
+[	 ]*[0-9a-f]+:[	 ]+41 03 04 00[	 ]+add[	 ]+\(%r8,%rax(,1)?\),%eax
+[	 ]*[0-9a-f]+:[	 ]+42 03 04 00[	 ]+add[	 ]+\(%rax,%r8(,1)?\),%eax
+[	 ]*[0-9a-f]+:[	 ]+49 03 04 00[	 ]+add[	 ]+\(%r8,%rax(,1)?\),%rax
+[	 ]*[0-9a-f]+:[	 ]+46 03 04 00[	 ]+add[	 ]+\(%rax,%r8(,1)?\),%r8d
+[	 ]*[0-9a-f]+:[	 ]+45 03 04 00[	 ]+add[	 ]+\(%r8,%rax(,1)?\),%r8d
+[	 ]*[0-9a-f]+:[	 ]+4a 03 04 00[	 ]+add[	 ]+\(%rax,%r8(,1)?\),%rax
 [	 ]*[0-9a-f]+:[	 ]+41\s+rex\.B
 [	 ]*[0-9a-f]+:[	 ]+9b dd 30\s+fsave\s+\(%rax\)
 [	 ]*[0-9a-f]+:[	 ]+9b 41 dd 30\s+fsave\s+\(%r8\)
--- a/gas/testsuite/gas/i386/rex.d
+++ b/gas/testsuite/gas/i386/rex.d
@@ -15,6 +15,14 @@  Disassembly of section .text:
 [	 ]*[0-9a-f]+:[	 ]+4a 0f ae 04 05 00 00 00 00[	 ]+fxsave64[	 ]+(0x0)?\(,%r8(,1)?\)
 [	 ]*[0-9a-f]+:[	 ]+43 0f ae 04 00[	 ]+fxsave[	 ]+\(%r8,%r8(,1)?\)
 [	 ]*[0-9a-f]+:[	 ]+4b 0f ae 04 00[	 ]+fxsave64[	 ]+\(%r8,%r8(,1)?\)
+[	 ]*[0-9a-f]+:[	 ]+48 03 04 00[	 ]+add[	 ]+\(%rax,%rax(,1)?\),%rax
+[	 ]*[0-9a-f]+:[	 ]+44 03 04 00[	 ]+add[	 ]+\(%rax,%rax(,1)?\),%r8d
+[	 ]*[0-9a-f]+:[	 ]+41 03 04 00[	 ]+add[	 ]+\(%r8,%rax(,1)?\),%eax
+[	 ]*[0-9a-f]+:[	 ]+42 03 04 00[	 ]+add[	 ]+\(%rax,%r8(,1)?\),%eax
+[	 ]*[0-9a-f]+:[	 ]+49 03 04 00[	 ]+add[	 ]+\(%r8,%rax(,1)?\),%rax
+[	 ]*[0-9a-f]+:[	 ]+46 03 04 00[	 ]+add[	 ]+\(%rax,%r8(,1)?\),%r8d
+[	 ]*[0-9a-f]+:[	 ]+45 03 04 00[	 ]+add[	 ]+\(%r8,%rax(,1)?\),%r8d
+[	 ]*[0-9a-f]+:[	 ]+4a 03 04 00[	 ]+add[	 ]+\(%rax,%r8(,1)?\),%rax
 [	 ]*[0-9a-f]+:[	 ]+41\s+rex\.B
 [	 ]*[0-9a-f]+:[	 ]+9b dd 30\s+fsave\s+\(%rax\)
 [	 ]*[0-9a-f]+:[	 ]+9b 41 dd 30\s+fsave\s+\(%r8\)
--- a/gas/testsuite/gas/i386/rex.s
+++ b/gas/testsuite/gas/i386/rex.s
@@ -10,6 +10,16 @@  _start:
 	rex/fxsave (%r8,%r8)
 	rex64/fxsave (%r8,%r8)
 
+	rex.w add	(%rax,%rax), %eax
+	rex.r add	(%rax,%rax), %eax
+	rex.b add	(%rax,%rax), %eax
+	rex.x add	(%rax,%rax), %eax
+
+	rex.w add	(%r8,%rax), %eax
+	rex.r add	(%rax,%r8), %eax
+	rex.b add	(%rax,%rax), %r8d
+	rex.x add	(%rax,%rax), %rax
+
 	.byte 0x41,0x9b,0xdd,0x30
 	fsave (%r8)