x86: Properly set YMM/ZMM features

Message ID CAMe9rOoGYq0=4txve-eN83gENiTfnTX4hNeQaAitz4=4O+Xviw@mail.gmail.com
State New
Headers show
Series
  • x86: Properly set YMM/ZMM features
Related show

Commit Message

Jose E. Marchesi via Binutils July 9, 2020, 4:17 p.m.
On Thu, Jul 9, 2020 at 9:13 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 09.07.2020 17:44, H.J. Lu wrote:

> > On Thu, Jul 9, 2020 at 8:36 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 09.07.2020 17:22, H.J. Lu wrote:

> >>> On Thu, Jul 9, 2020 at 8:21 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>

> >>>> On 09.07.2020 17:02, H.J. Lu via Binutils wrote:

> >>>>>       * configure.ac: Configure with --enable-x86-used-note by default

> >>>>>       for Linux/x86.

> >>>>

> >>>> I'm quite unconvinced this is a good idea as long as the contents of

> >>>> these notes are neither reliable nor properly settled on what they

> >>>> actually mean. This should be considered an experimental feature for

> >>>> now, and hence would better not be enabled by default to avoid

> >>>> future backwards compatibility issues.

> >>>>

> >>>

> >>> It has been an experimental feature for 2 years.

> >>

> >> And when I asked questions about the underlying spec I did get at

> >> best fuzzy answers from you. I still have on my todo list to get

> >> this in shape with a pretty recent reply of yours, which was

> >> supporting my view on how things should be (and why things are

> >> broken right now), and hence somewhat contrary to earlier replies

> >> of yours.

> >>

> >> To just name the most unclear (to me) aspect of what's there

> >> currently: What's the distinction between xmm, ymm, and zmm, when

> >> zmm is a superset of ymm (and ymm one of xmm), yet at the same

> >> time AVX512 writes to xmm mean writes to ymm and zmm (zeroing

> >> upper parts) as well. From my observations e.g. an AVX512 insn

> >> accessing just xmm registers will record just an xmm dependency,

> >> which then is simply wrong - the resulting binary in fact depends

> >> on all of xmm, ymm, zmm, and the mask registers. IIRC I proposed

> >> back then to tie what an insn records to the XCR0 bits it

> >> requires to be set in order for it to not fault.

> >

> > Should we add a field to i386 opcode table for this?

>

> I was wanting to try without, to see how much logic would be needed.

> If it gets too much, doing so may be the better choice.


I am checking in this patch.

> >>>  Let's see what fallouts will be.

> >>

> >> Fallout may become noticeable only when the behavior of the notes

> >> changes: People may start noticing that new (correct) dependencies

> >> prevent things from working (assuming there's any consumer of

> >> these notes).

> >

> > We have a couple months to address this before 2.36 is released.

>

> Under the assumption that people test non-released code ...

>


We will see.


-- 
H.J.

Comments

Jose E. Marchesi via Binutils July 9, 2020, 4:22 p.m. | #1
On Thu, Jul 9, 2020 at 9:17 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Thu, Jul 9, 2020 at 9:13 AM Jan Beulich <jbeulich@suse.com> wrote:

> >

> > On 09.07.2020 17:44, H.J. Lu wrote:

> > > On Thu, Jul 9, 2020 at 8:36 AM Jan Beulich <jbeulich@suse.com> wrote:

> > >>

> > >> On 09.07.2020 17:22, H.J. Lu wrote:

> > >>> On Thu, Jul 9, 2020 at 8:21 AM Jan Beulich <jbeulich@suse.com> wrote:

> > >>>>

> > >>>> On 09.07.2020 17:02, H.J. Lu via Binutils wrote:

> > >>>>>       * configure.ac: Configure with --enable-x86-used-note by default

> > >>>>>       for Linux/x86.

> > >>>>

> > >>>> I'm quite unconvinced this is a good idea as long as the contents of

> > >>>> these notes are neither reliable nor properly settled on what they

> > >>>> actually mean. This should be considered an experimental feature for

> > >>>> now, and hence would better not be enabled by default to avoid

> > >>>> future backwards compatibility issues.

> > >>>>

> > >>>

> > >>> It has been an experimental feature for 2 years.

> > >>

> > >> And when I asked questions about the underlying spec I did get at

> > >> best fuzzy answers from you. I still have on my todo list to get

> > >> this in shape with a pretty recent reply of yours, which was

> > >> supporting my view on how things should be (and why things are

> > >> broken right now), and hence somewhat contrary to earlier replies

> > >> of yours.

> > >>

> > >> To just name the most unclear (to me) aspect of what's there

> > >> currently: What's the distinction between xmm, ymm, and zmm, when

> > >> zmm is a superset of ymm (and ymm one of xmm), yet at the same

> > >> time AVX512 writes to xmm mean writes to ymm and zmm (zeroing

> > >> upper parts) as well. From my observations e.g. an AVX512 insn

> > >> accessing just xmm registers will record just an xmm dependency,

> > >> which then is simply wrong - the resulting binary in fact depends

> > >> on all of xmm, ymm, zmm, and the mask registers. IIRC I proposed

> > >> back then to tie what an insn records to the XCR0 bits it

> > >> requires to be set in order for it to not fault.

> > >

> > > Should we add a field to i386 opcode table for this?

> >

> > I was wanting to try without, to see how much logic would be needed.

> > If it gets too much, doing so may be the better choice.

>

> I am checking in this patch.


Here is the right patch.

> > >>>  Let's see what fallouts will be.

> > >>

> > >> Fallout may become noticeable only when the behavior of the notes

> > >> changes: People may start noticing that new (correct) dependencies

> > >> prevent things from working (assuming there's any consumer of

> > >> these notes).

> > >

> > > We have a couple months to address this before 2.36 is released.

> >

> > Under the assumption that people test non-released code ...

> >

>

> We will see.

>

>

> --

> H.J.




-- 
H.J.
From 1f9db299bf5cf8d6b84336204c888e417ca60f47 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 9 Jul 2020 09:14:09 -0700
Subject: [PATCH] x86: Properly set YMM/ZMM features

Since VEX/EVEX vector instructions will always update the full YMM/ZMM
registers, set YMM/ZMM features for VEX/EVEX vector instructions.

	* config/tc-i386.c (output_insn): Set YMM/ZMM features for
	VEX/EVEX vector instructions.
	* testsuite/gas/i386/property-4.d: New file.
	* testsuite/gas/i386/property-4.s: Likewise.
	* testsuite/gas/i386/property-5.d: Likewise.
	* testsuite/gas/i386/property-5.s: Likewise.
	* testsuite/gas/i386/x86-64-property-4.d: Likewise.
	* testsuite/gas/i386/x86-64-property-5.d: Likewise.
---
 gas/config/tc-i386.c                       |  9 +++++++--
 gas/testsuite/gas/i386/i386.exp            |  4 ++++
 gas/testsuite/gas/i386/property-4.d        |  9 +++++++++
 gas/testsuite/gas/i386/property-4.s        |  2 ++
 gas/testsuite/gas/i386/property-5.d        |  9 +++++++++
 gas/testsuite/gas/i386/property-5.s        |  2 ++
 gas/testsuite/gas/i386/x86-64-property-4.d | 10 ++++++++++
 gas/testsuite/gas/i386/x86-64-property-5.d | 10 ++++++++++
 8 files changed, 53 insertions(+), 2 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/property-4.d
 create mode 100644 gas/testsuite/gas/i386/property-4.s
 create mode 100644 gas/testsuite/gas/i386/property-5.d
 create mode 100644 gas/testsuite/gas/i386/property-5.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-property-4.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-property-5.d

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 2e0eb24753..bb51133ecd 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -9120,9 +9120,14 @@ output_insn (void)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MMX;
       if (i.has_regxmm)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XMM;
-      if (i.has_regymm)
+      if (i.has_regymm
+	  || (i.has_regxmm
+	      && (i.tm.opcode_modifier.vex
+		  || i.tm.opcode_modifier.evex)))
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_YMM;
-      if (i.has_regzmm)
+      if (i.has_regzmm
+	  || ((i.has_regxmm || i.has_regymm)
+	      && i.tm.opcode_modifier.evex))
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_ZMM;
       if (i.tm.cpu_flags.bitfield.cpufxsr)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_FXSR;
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index 37aa39698c..0074a13692 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -622,6 +622,8 @@ if [expr ([istarget "i*86-*-*"] ||  [istarget "x86_64-*-*"]) && [gas_32_check]]
 	run_dump_test "property-1"
 	run_dump_test "property-2"
 	run_dump_test "property-3"
+	run_dump_test "property-4"
+	run_dump_test "property-5"
 
 	if {[istarget "*-*-linux*"]} then {
 	    run_dump_test "align-branch-3"
@@ -1207,6 +1209,8 @@ if [expr ([istarget "i*86-*-*"] || [istarget "x86_64-*-*"]) && [gas_64_check]] t
 	run_dump_test "x86-64-property-1"
 	run_dump_test "x86-64-property-2"
 	run_dump_test "x86-64-property-3"
+	run_dump_test "x86-64-property-4"
+	run_dump_test "x86-64-property-5"
 
 	if {[istarget "*-*-linux*"]} then {
 	    run_dump_test "x86-64-align-branch-3"
diff --git a/gas/testsuite/gas/i386/property-4.d b/gas/testsuite/gas/i386/property-4.d
new file mode 100644
index 0000000000..0fe6bc7db4
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-4.d
@@ -0,0 +1,9 @@
+#name: i386 property 4
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX
+	x86 feature used: x86, XMM, YMM
diff --git a/gas/testsuite/gas/i386/property-4.s b/gas/testsuite/gas/i386/property-4.s
new file mode 100644
index 0000000000..dcc28826a4
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-4.s
@@ -0,0 +1,2 @@
+	.text
+	{vex} vmovaps %xmm0, %xmm0
diff --git a/gas/testsuite/gas/i386/property-5.d b/gas/testsuite/gas/i386/property-5.d
new file mode 100644
index 0000000000..fc6a5181d1
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-5.d
@@ -0,0 +1,9 @@
+#name: i386 property 4
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX512F
+	x86 feature used: x86, XMM, YMM, ZMM
diff --git a/gas/testsuite/gas/i386/property-5.s b/gas/testsuite/gas/i386/property-5.s
new file mode 100644
index 0000000000..ff8ce48db9
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-5.s
@@ -0,0 +1,2 @@
+	.text
+	{evex} vmovaps %xmm0, %xmm0
diff --git a/gas/testsuite/gas/i386/x86-64-property-4.d b/gas/testsuite/gas/i386/x86-64-property-4.d
new file mode 100644
index 0000000000..4a489509d6
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-property-4.d
@@ -0,0 +1,10 @@
+#name: x86-64 property 4
+#source: property-4.s
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX
+	x86 feature used: x86, XMM, YMM
diff --git a/gas/testsuite/gas/i386/x86-64-property-5.d b/gas/testsuite/gas/i386/x86-64-property-5.d
new file mode 100644
index 0000000000..590d8d585f
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-property-5.d
@@ -0,0 +1,10 @@
+#name: x86-64 property 5
+#source: property-5.s
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX512F
+	x86 feature used: x86, XMM, YMM, ZMM
Jan Beulich July 10, 2020, 7:02 a.m. | #2
On 09.07.2020 18:22, H.J. Lu wrote:
> On Thu, Jul 9, 2020 at 9:17 AM H.J. Lu <hjl.tools@gmail.com> wrote:

>> On Thu, Jul 9, 2020 at 9:13 AM Jan Beulich <jbeulich@suse.com> wrote:

>>> On 09.07.2020 17:44, H.J. Lu wrote:

>>>> Should we add a field to i386 opcode table for this?

>>>

>>> I was wanting to try without, to see how much logic would be needed.

>>> If it gets too much, doing so may be the better choice.

>>

>> I am checking in this patch.

> 

> Here is the right patch.


Well, this is certainly an improvement. But it doesn't come close to
what's needed. Again - imo the concept of i.has_reg* isn't helpful
at all. It's not the registers an insn _actually_ accesses, but the
ones the template says it might access. Think of vfpclass* with a
memory operand, for example. Or see some of the extra checks of
certain opcodes when setting the MMX bit, which could be avoided by
using such an alternative model.

I'm leaving aside here special cases where the templates don't carry
enough information. I'm also leaving aside the need to e.g. record
mask register use in some form (maybe translating to ZMM use if you
want to avoid introducing a separate tracking bit), as well as
various other issues I had spotted without looking very closely yet.

I believe a fundamental issue here is the very scarce testing: While
it may be avoidable to add an attribute to the insn templates, there
should be full testing coverage of _all_ insns with _all_ possible
operand type combinations, if the resulting notes are to be
dependable. If such (presumably table based) testing was there (and
of course if the table reflected the real needs rather than what
gas currently happens to produce), you'd easily notice the issues
I've spotted (and likely more).

Jan

Patch

From 9237422e149620927a8f19ab58ebba946ac8f4b6 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 9 Jul 2020 09:14:09 -0700
Subject: [PATCH] x86: Properly set YMM/ZMM features

Since VEX/EVEX vector instructions will always update the full YMM/ZMM
registers, set YMM/ZMM features for VEX/EVEX vector instructions.

	* testsuite/gas/i386/property-4.d: New file.
	* testsuite/gas/i386/property-4.s: Likewise.
	* testsuite/gas/i386/property-5.d: Likewise.
	* testsuite/gas/i386/property-5.s: Likewise.
	* testsuite/gas/i386/x86-64-property-4.d: Likewise.
	* testsuite/gas/i386/x86-64-property-5.d: Likewise.
---
 gas/config/tc-i386.c                       |  6 ++++--
 gas/testsuite/gas/i386/i386.exp            |  4 ++++
 gas/testsuite/gas/i386/property-4.d        |  9 +++++++++
 gas/testsuite/gas/i386/property-4.s        |  2 ++
 gas/testsuite/gas/i386/property-5.d        |  9 +++++++++
 gas/testsuite/gas/i386/property-5.s        |  2 ++
 gas/testsuite/gas/i386/x86-64-property-4.d | 10 ++++++++++
 gas/testsuite/gas/i386/x86-64-property-5.d | 10 ++++++++++
 8 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/property-4.d
 create mode 100644 gas/testsuite/gas/i386/property-4.s
 create mode 100644 gas/testsuite/gas/i386/property-5.d
 create mode 100644 gas/testsuite/gas/i386/property-5.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-property-4.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-property-5.d

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 2e0eb24753..6404bf1a6a 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -9120,9 +9120,11 @@  output_insn (void)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MMX;
       if (i.has_regxmm)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XMM;
-      if (i.has_regymm)
+      if (i.has_regymm
+	  || i.tm.opcode_modifier.vex
+	  || i.tm.opcode_modifier.evex)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_YMM;
-      if (i.has_regzmm)
+      if (i.has_regzmm || i.tm.opcode_modifier.evex)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_ZMM;
       if (i.tm.cpu_flags.bitfield.cpufxsr)
 	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_FXSR;
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index 37aa39698c..0074a13692 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -622,6 +622,8 @@  if [expr ([istarget "i*86-*-*"] ||  [istarget "x86_64-*-*"]) && [gas_32_check]]
 	run_dump_test "property-1"
 	run_dump_test "property-2"
 	run_dump_test "property-3"
+	run_dump_test "property-4"
+	run_dump_test "property-5"
 
 	if {[istarget "*-*-linux*"]} then {
 	    run_dump_test "align-branch-3"
@@ -1207,6 +1209,8 @@  if [expr ([istarget "i*86-*-*"] || [istarget "x86_64-*-*"]) && [gas_64_check]] t
 	run_dump_test "x86-64-property-1"
 	run_dump_test "x86-64-property-2"
 	run_dump_test "x86-64-property-3"
+	run_dump_test "x86-64-property-4"
+	run_dump_test "x86-64-property-5"
 
 	if {[istarget "*-*-linux*"]} then {
 	    run_dump_test "x86-64-align-branch-3"
diff --git a/gas/testsuite/gas/i386/property-4.d b/gas/testsuite/gas/i386/property-4.d
new file mode 100644
index 0000000000..0fe6bc7db4
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-4.d
@@ -0,0 +1,9 @@ 
+#name: i386 property 4
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX
+	x86 feature used: x86, XMM, YMM
diff --git a/gas/testsuite/gas/i386/property-4.s b/gas/testsuite/gas/i386/property-4.s
new file mode 100644
index 0000000000..dcc28826a4
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-4.s
@@ -0,0 +1,2 @@ 
+	.text
+	{vex} vmovaps %xmm0, %xmm0
diff --git a/gas/testsuite/gas/i386/property-5.d b/gas/testsuite/gas/i386/property-5.d
new file mode 100644
index 0000000000..fc6a5181d1
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-5.d
@@ -0,0 +1,9 @@ 
+#name: i386 property 4
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX512F
+	x86 feature used: x86, XMM, YMM, ZMM
diff --git a/gas/testsuite/gas/i386/property-5.s b/gas/testsuite/gas/i386/property-5.s
new file mode 100644
index 0000000000..ff8ce48db9
--- /dev/null
+++ b/gas/testsuite/gas/i386/property-5.s
@@ -0,0 +1,2 @@ 
+	.text
+	{evex} vmovaps %xmm0, %xmm0
diff --git a/gas/testsuite/gas/i386/x86-64-property-4.d b/gas/testsuite/gas/i386/x86-64-property-4.d
new file mode 100644
index 0000000000..4a489509d6
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-property-4.d
@@ -0,0 +1,10 @@ 
+#name: x86-64 property 4
+#source: property-4.s
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX
+	x86 feature used: x86, XMM, YMM
diff --git a/gas/testsuite/gas/i386/x86-64-property-5.d b/gas/testsuite/gas/i386/x86-64-property-5.d
new file mode 100644
index 0000000000..590d8d585f
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-property-5.d
@@ -0,0 +1,10 @@ 
+#name: x86-64 property 5
+#source: property-5.s
+#as: -mx86-used-note=yes --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 ISA used: AVX512F
+	x86 feature used: x86, XMM, YMM, ZMM
-- 
2.26.2