public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Binutils <binutils@sourceware.org>
Cc: "H.J. Lu" <hjl.tools@gmail.com>
Subject: [PATCH 1/3] x86: correct cpu_arch_isa_flags maintenance
Date: Fri, 15 Sep 2023 10:59:15 +0200	[thread overview]
Message-ID: <cac1fe52-8b04-e869-0243-6ca4fa07864c@suse.com> (raw)
In-Reply-To: <8d21b532-7a4a-da25-3e95-d1aea76f75a3@suse.com>

These may not be set from a value derived from cpu_arch_flags: That
starts with (almost) all functionality enabled, while cpu_arch_isa_flags
is supposed to track features that were explicitly enabled (and perhaps
later disabled) by the user.

To avoid needing to do any such adjustment in two places (each),
introduce helper functions used by both command line handling and
directive processing.
---
While setting of vector_size could be moved into isa_disable() (further
reducing code duplication), the same isn't true for isa_enable().
Because of the asymmetry I didn't do so. Thoughts / opinions?

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2804,13 +2804,41 @@ check_cpu_arch_compatible (const char *n
 }
 
 static void
-extend_cpu_sub_arch_name (const char *name)
+extend_cpu_sub_arch_name (const char *pfx, const char *name)
 {
   if (cpu_sub_arch_name)
     cpu_sub_arch_name = reconcat (cpu_sub_arch_name, cpu_sub_arch_name,
-				  ".", name, (const char *) NULL);
+				  pfx, name, (const char *) NULL);
   else
-    cpu_sub_arch_name = concat (".", name, (const char *) NULL);
+    cpu_sub_arch_name = concat (pfx, name, (const char *) NULL);
+}
+
+static void isa_enable (unsigned int idx)
+{
+  i386_cpu_flags flags = cpu_flags_or (cpu_arch_flags, cpu_arch[idx].enable);
+
+  if (!cpu_flags_equal (&flags, &cpu_arch_flags))
+    {
+      extend_cpu_sub_arch_name (".", cpu_arch[idx].name);
+      cpu_arch_flags = flags;
+    }
+
+  cpu_arch_isa_flags = cpu_flags_or (cpu_arch_isa_flags, cpu_arch[idx].enable);
+}
+
+static void isa_disable (unsigned int idx)
+{
+  i386_cpu_flags flags
+    = cpu_flags_and_not (cpu_arch_flags, cpu_arch[idx].disable);
+
+  if (!cpu_flags_equal (&flags, &cpu_arch_flags))
+    {
+      extend_cpu_sub_arch_name (".no", cpu_arch[idx].name);
+      cpu_arch_flags = flags;
+    }
+
+  cpu_arch_isa_flags
+    = cpu_flags_and_not (cpu_arch_isa_flags, cpu_arch[idx].disable);
 }
 
 static void
@@ -2834,7 +2862,6 @@ set_cpu_arch (int dummy ATTRIBUTE_UNUSED
   int e;
   const char *string;
   unsigned int j = 0;
-  i386_cpu_flags flags;
 
   SKIP_WHITESPACE ();
 
@@ -2987,17 +3014,7 @@ set_cpu_arch (int dummy ATTRIBUTE_UNUSED
 	  if (cpu_flags_all_zero (&cpu_arch[j].enable))
 	    continue;
 
-	  flags = cpu_flags_or (cpu_arch_flags, cpu_arch[j].enable);
-
-	  if (!cpu_flags_equal (&flags, &cpu_arch_flags))
-	    {
-	      extend_cpu_sub_arch_name (string + 1);
-	      cpu_arch_flags = flags;
-	      cpu_arch_isa_flags = flags;
-	    }
-	  else
-	    cpu_arch_isa_flags
-	      = cpu_flags_or (cpu_arch_isa_flags, cpu_arch[j].enable);
+	  isa_enable (j);
 
 	  (void) restore_line_pointer (e);
 
@@ -3044,13 +3061,7 @@ set_cpu_arch (int dummy ATTRIBUTE_UNUSED
 	if (cpu_arch[j].type == PROCESSOR_NONE
 	    && strcmp (string + 3, cpu_arch[j].name) == 0)
 	  {
-	    flags = cpu_flags_and_not (cpu_arch_flags, cpu_arch[j].disable);
-	    if (!cpu_flags_equal (&flags, &cpu_arch_flags))
-	      {
-		extend_cpu_sub_arch_name (string + 1);
-		cpu_arch_flags = flags;
-		cpu_arch_isa_flags = flags;
-	      }
+	    isa_disable (j);
 
 	    if (cpu_arch[j].vsz == vsz_set)
 	      vector_size = VSZ_DEFAULT;
@@ -14598,21 +14609,7 @@ md_parse_option (int c, const char *arg)
 		       && !cpu_flags_all_zero (&cpu_arch[j].enable))
 		{
 		  /* ISA extension.  */
-		  i386_cpu_flags flags;
-
-		  flags = cpu_flags_or (cpu_arch_flags,
-					cpu_arch[j].enable);
-
-		  if (!cpu_flags_equal (&flags, &cpu_arch_flags))
-		    {
-		      extend_cpu_sub_arch_name (arch);
-		      cpu_arch_flags = flags;
-		      cpu_arch_isa_flags = flags;
-		    }
-		  else
-		    cpu_arch_isa_flags
-		      = cpu_flags_or (cpu_arch_isa_flags,
-				      cpu_arch[j].enable);
+		  isa_enable (j);
 
 		  switch (cpu_arch[j].vsz)
 		    {
@@ -14655,16 +14652,7 @@ md_parse_option (int c, const char *arg)
 		if (cpu_arch[j].type == PROCESSOR_NONE
 		    && strcmp (arch + 2, cpu_arch[j].name) == 0)
 		  {
-		    i386_cpu_flags flags;
-
-		    flags = cpu_flags_and_not (cpu_arch_flags,
-					       cpu_arch[j].disable);
-		    if (!cpu_flags_equal (&flags, &cpu_arch_flags))
-		      {
-			extend_cpu_sub_arch_name (arch);
-			cpu_arch_flags = flags;
-			cpu_arch_isa_flags = flags;
-		      }
+		    isa_disable (j);
 		    if (cpu_arch[j].vsz == vsz_set)
 		      vector_size = VSZ_DEFAULT;
 		    break;
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -147,6 +147,7 @@ if [gas_32_check] then {
     run_dump_test "nops-6"
     run_dump_test "nops-7"
     run_dump_test "nops-8"
+    run_dump_test "nops-9"
     run_dump_test "noreg16"
     run_list_test "noreg16"
     run_dump_test "noreg16-data32"
--- /dev/null
+++ b/gas/testsuite/gas/i386/nops-9.d
@@ -0,0 +1,28 @@
+#objdump: -drw
+#name: i386 nops 9
+
+.*: +file format .*
+
+Disassembly of section .text:
+
+0+ <default>:
+[ 	]*[a-f0-9]+:	0f be f0             	movsbl %al,%esi
+[ 	]*[a-f0-9]+:	8d b4 26 00 00 00 00 	lea    0x0\(%esi,%eiz,1\),%esi
+[ 	]*[a-f0-9]+:	8d b6 00 00 00 00    	lea    0x0\(%esi\),%esi
+
+0+10 <nopopcnt>:
+[ 	]*[a-f0-9]+:	0f be f0             	movsbl %al,%esi
+[ 	]*[a-f0-9]+:	8d b4 26 00 00 00 00 	lea    0x0\(%esi,%eiz,1\),%esi
+[ 	]*[a-f0-9]+:	8d b6 00 00 00 00    	lea    0x0\(%esi\),%esi
+
+0+20 <popcnt>:
+[ 	]*[a-f0-9]+:	f3 0f b8 f0          	popcnt %eax,%esi
+[ 	]*[a-f0-9]+:	8d b4 26 00 00 00 00 	lea    0x0\(%esi,%eiz,1\),%esi
+[ 	]*[a-f0-9]+:	8d 74 26 00          	lea    0x0\(%esi,%eiz,1\),%esi
+[ 	]*[a-f0-9]+:	90                   	nop
+
+0+30 <nop>:
+[ 	]*[a-f0-9]+:	0f be f0             	movsbl %al,%esi
+[ 	]*[a-f0-9]+:	66 66 2e 0f 1f 84 00 00 00 00 00 	data16 nopw %cs:0x0\(%eax,%eax,1\)
+[ 	]*[a-f0-9]+:	66 90                	xchg   %ax,%ax
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/i386/nops-9.s
@@ -0,0 +1,19 @@
+	.text
+default:
+	movsbl %al,%esi
+	.p2align 4
+
+	.arch .nopopcnt
+nopopcnt:
+	movsbl %al,%esi
+	.p2align 4
+
+	.arch .popcnt
+popcnt:
+	popcnt %eax,%esi
+	.p2align 4
+
+	.arch .nop
+nop:
+	movsbl %al,%esi
+	.p2align 4


  reply	other threads:[~2023-09-15  8:59 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-15  8:58 [PATCH 0/3] x86: improve encoding selection and prereq tidying Jan Beulich
2023-09-15  8:59 ` Jan Beulich [this message]
2023-09-15  8:59 ` [PATCH 2/3] x86: drop cpu_arch_tune_flags Jan Beulich
2023-09-15  9:00 ` [PATCH 3/3] x86: prefer VEX encodings over EVEX ones when possible Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cac1fe52-8b04-e869-0243-6ca4fa07864c@suse.com \
    --to=jbeulich@suse.com \
    --cc=binutils@sourceware.org \
    --cc=hjl.tools@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).