x86: imply all No_*Suf when none is set in a template

Message ID 2e594028-7f60-a0b9-086b-269d883c7441@suse.com
State New
Headers show
Series
  • x86: imply all No_*Suf when none is set in a template
Related show

Commit Message

Jan Beulich Nov. 14, 2019, 8:51 a.m.
Since no template would ever allow for none of them to be set, reduce
table size and improve readability and hence maintainability by implying
all of them to be set when a template specifies none.

opcodes/
2019-11-XX  Jan Beulich  <jbeulich@suse.com>

	* i386-gen.c (process_i386_opcode_modifier): Widen scope of
	bwlq_suf. New local variable other_suf. Emit warning when all
	No_*Suf are set. Set all No_*Suf when none are set.
	* i386-opc.tbl: Drop No_bSuf, No_wSuf, No_lSuf, No_sSuf,
	No_qSuf, and No_ldSuf when they're all specified at the same
	time.
---
Full patch (including entirely mechanical i386-opc.tbl changes)
attached in compressed form, for size reasons. There's no change
to any of the generated files.

Comments

H.J. Lu Nov. 14, 2019, 7:26 p.m. | #1
On Thu, Nov 14, 2019 at 12:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> Since no template would ever allow for none of them to be set, reduce

> table size and improve readability and hence maintainability by implying

> all of them to be set when a template specifies none.

>

> opcodes/

> 2019-11-XX  Jan Beulich  <jbeulich@suse.com>

>

>         * i386-gen.c (process_i386_opcode_modifier): Widen scope of

>         bwlq_suf. New local variable other_suf. Emit warning when all

>         No_*Suf are set. Set all No_*Suf when none are set.

>         * i386-opc.tbl: Drop No_bSuf, No_wSuf, No_lSuf, No_sSuf,

>         No_qSuf, and No_ldSuf when they're all specified at the same

>         time.


I don't think it may the table more readable.   Why not simply add

#define  No_Suf  No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf

and use it?

-- 
H.J.
Jan Beulich Nov. 15, 2019, 8:35 a.m. | #2
On 14.11.2019 20:26, H.J. Lu wrote:
> On Thu, Nov 14, 2019 at 12:51 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> Since no template would ever allow for none of them to be set, reduce

>> table size and improve readability and hence maintainability by implying

>> all of them to be set when a template specifies none.

>>

>> opcodes/

>> 2019-11-XX  Jan Beulich  <jbeulich@suse.com>

>>

>>         * i386-gen.c (process_i386_opcode_modifier): Widen scope of

>>         bwlq_suf. New local variable other_suf. Emit warning when all

>>         No_*Suf are set. Set all No_*Suf when none are set.

>>         * i386-opc.tbl: Drop No_bSuf, No_wSuf, No_lSuf, No_sSuf,

>>         No_qSuf, and No_ldSuf when they're all specified at the same

>>         time.

> 

> I don't think it may the table more readable.   Why not simply add

> 

> #define  No_Suf  No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf

> 

> and use it?


Because this would require yet another global change once we switch
to positive identification of the permitted suffixes (which should
have been done from the beginning imo). Since the set of suffixes
that can go together is pretty limited (i.e. a fair subset of the
combinations currently possible to specify do never occur), my plan
is to switch to an enumeration here, containing enumerators like
bwlqSuf or wlqSuf.

Jan
H.J. Lu Nov. 15, 2019, 6:05 p.m. | #3
On Fri, Nov 15, 2019 at 12:35 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 14.11.2019 20:26, H.J. Lu wrote:

> > On Thu, Nov 14, 2019 at 12:51 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> Since no template would ever allow for none of them to be set, reduce

> >> table size and improve readability and hence maintainability by implying

> >> all of them to be set when a template specifies none.

> >>

> >> opcodes/

> >> 2019-11-XX  Jan Beulich  <jbeulich@suse.com>

> >>

> >>         * i386-gen.c (process_i386_opcode_modifier): Widen scope of

> >>         bwlq_suf. New local variable other_suf. Emit warning when all

> >>         No_*Suf are set. Set all No_*Suf when none are set.

> >>         * i386-opc.tbl: Drop No_bSuf, No_wSuf, No_lSuf, No_sSuf,

> >>         No_qSuf, and No_ldSuf when they're all specified at the same

> >>         time.

> >

> > I don't think it may the table more readable.   Why not simply add

> >

> > #define  No_Suf  No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf

> >

> > and use it?

>

> Because this would require yet another global change once we switch

> to positive identification of the permitted suffixes (which should

> have been done from the beginning imo). Since the set of suffixes

> that can go together is pretty limited (i.e. a fair subset of the

> combinations currently possible to specify do never occur), my plan

> is to switch to an enumeration here, containing enumerators like

> bwlqSuf or wlqSuf.

>


Replace or remove No_Suf should be very easy.

-- 
H.J.
Jan Beulich Nov. 18, 2019, 1:09 p.m. | #4
On 15.11.2019 19:05,  H.J. Lu  wrote:
> On Fri, Nov 15, 2019 at 12:35 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 14.11.2019 20:26, H.J. Lu wrote:

>>> On Thu, Nov 14, 2019 at 12:51 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> Since no template would ever allow for none of them to be set, reduce

>>>> table size and improve readability and hence maintainability by implying

>>>> all of them to be set when a template specifies none.

>>>>

>>>> opcodes/

>>>> 2019-11-XX  Jan Beulich  <jbeulich@suse.com>

>>>>

>>>>         * i386-gen.c (process_i386_opcode_modifier): Widen scope of

>>>>         bwlq_suf. New local variable other_suf. Emit warning when all

>>>>         No_*Suf are set. Set all No_*Suf when none are set.

>>>>         * i386-opc.tbl: Drop No_bSuf, No_wSuf, No_lSuf, No_sSuf,

>>>>         No_qSuf, and No_ldSuf when they're all specified at the same

>>>>         time.

>>>

>>> I don't think it may the table more readable.   Why not simply add

>>>

>>> #define  No_Suf  No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf

>>>

>>> and use it?

>>

>> Because this would require yet another global change once we switch

>> to positive identification of the permitted suffixes (which should

>> have been done from the beginning imo). Since the set of suffixes

>> that can go together is pretty limited (i.e. a fair subset of the

>> combinations currently possible to specify do never occur), my plan

>> is to switch to an enumeration here, containing enumerators like

>> bwlqSuf or wlqSuf.

> 

> Replace or remove No_Suf should be very easy.


"easy" is not a category of my thinking here. It's the huge amount
of code churn and the resulting unless-ness of e.g. "git blame" that
I'd like to avoid. Dropping the code again that this patch adds to
i386-gen.c is also "easy" enough.

Jan
H.J. Lu Nov. 22, 2019, 7:35 a.m. | #5
On Mon, Nov 18, 2019 at 5:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 15.11.2019 19:05,  H.J. Lu  wrote:

> > On Fri, Nov 15, 2019 at 12:35 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 14.11.2019 20:26, H.J. Lu wrote:

> >>> On Thu, Nov 14, 2019 at 12:51 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>

> >>>> Since no template would ever allow for none of them to be set, reduce

> >>>> table size and improve readability and hence maintainability by implying

> >>>> all of them to be set when a template specifies none.

> >>>>

> >>>> opcodes/

> >>>> 2019-11-XX  Jan Beulich  <jbeulich@suse.com>

> >>>>

> >>>>         * i386-gen.c (process_i386_opcode_modifier): Widen scope of

> >>>>         bwlq_suf. New local variable other_suf. Emit warning when all

> >>>>         No_*Suf are set. Set all No_*Suf when none are set.

> >>>>         * i386-opc.tbl: Drop No_bSuf, No_wSuf, No_lSuf, No_sSuf,

> >>>>         No_qSuf, and No_ldSuf when they're all specified at the same

> >>>>         time.

> >>>

> >>> I don't think it may the table more readable.   Why not simply add

> >>>

> >>> #define  No_Suf  No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf

> >>>

> >>> and use it?

> >>

> >> Because this would require yet another global change once we switch

> >> to positive identification of the permitted suffixes (which should

> >> have been done from the beginning imo). Since the set of suffixes

> >> that can go together is pretty limited (i.e. a fair subset of the

> >> combinations currently possible to specify do never occur), my plan

> >> is to switch to an enumeration here, containing enumerators like

> >> bwlqSuf or wlqSuf.

> >

> > Replace or remove No_Suf should be very easy.

>

> "easy" is not a category of my thinking here. It's the huge amount

> of code churn and the resulting unless-ness of e.g. "git blame" that

> I'd like to avoid. Dropping the code again that this patch adds to

> i386-gen.c is also "easy" enough.

>


I don't think "git blame" is an issue here.  No_Suf makes clear
what exactly happens.


-- 
H.J.
Jan Beulich Nov. 22, 2019, 1:43 p.m. | #6
On 22.11.2019 08:35,  H.J. Lu  wrote:
> On Mon, Nov 18, 2019 at 5:10 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 15.11.2019 19:05,  H.J. Lu  wrote:

>>> On Fri, Nov 15, 2019 at 12:35 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> On 14.11.2019 20:26, H.J. Lu wrote:

>>>>> On Thu, Nov 14, 2019 at 12:51 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>>

>>>>>> Since no template would ever allow for none of them to be set, reduce

>>>>>> table size and improve readability and hence maintainability by implying

>>>>>> all of them to be set when a template specifies none.

>>>>>>

>>>>>> opcodes/

>>>>>> 2019-11-XX  Jan Beulich  <jbeulich@suse.com>

>>>>>>

>>>>>>         * i386-gen.c (process_i386_opcode_modifier): Widen scope of

>>>>>>         bwlq_suf. New local variable other_suf. Emit warning when all

>>>>>>         No_*Suf are set. Set all No_*Suf when none are set.

>>>>>>         * i386-opc.tbl: Drop No_bSuf, No_wSuf, No_lSuf, No_sSuf,

>>>>>>         No_qSuf, and No_ldSuf when they're all specified at the same

>>>>>>         time.

>>>>>

>>>>> I don't think it may the table more readable.   Why not simply add

>>>>>

>>>>> #define  No_Suf  No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf

>>>>>

>>>>> and use it?

>>>>

>>>> Because this would require yet another global change once we switch

>>>> to positive identification of the permitted suffixes (which should

>>>> have been done from the beginning imo). Since the set of suffixes

>>>> that can go together is pretty limited (i.e. a fair subset of the

>>>> combinations currently possible to specify do never occur), my plan

>>>> is to switch to an enumeration here, containing enumerators like

>>>> bwlqSuf or wlqSuf.

>>>

>>> Replace or remove No_Suf should be very easy.

>>

>> "easy" is not a category of my thinking here. It's the huge amount

>> of code churn and the resulting unless-ness of e.g. "git blame" that

>> I'd like to avoid. Dropping the code again that this patch adds to

>> i386-gen.c is also "easy" enough.

> 

> I don't think "git blame" is an issue here.  No_Suf makes clear

> what exactly happens.


Well, I've put the patch onto my "rejected" list, as I'm not going
to put my name under something that causes this massive amount of
unnecessary code churn. I may reactivate it once I find time to do
the full conversion that I had outlined.

Jan

Patch

--- a/opcodes/i386-gen.c
+++ b/opcodes/i386-gen.c
@@ -1089,6 +1089,7 @@  static void
 process_i386_opcode_modifier (FILE *table, char *mod, char **opnd, int lineno)
 {
   char *str, *next, *last;
+  unsigned int bwlq_suf = 0xf, other_suf = 3;
   bitfield modifiers [ARRAY_SIZE (opcode_modifiers)];
 
   active_isstring = 0;
@@ -1098,7 +1099,7 @@  process_i386_opcode_modifier (FILE *tabl
 
   if (strcmp (mod, "0"))
     {
-      unsigned int have_w = 0, bwlq_suf = 0xf;
+      unsigned int have_w = 0;
 
       last = mod + strlen (mod);
       for (next = mod; next && next < last; )
@@ -1125,6 +1126,10 @@  process_i386_opcode_modifier (FILE *tabl
 		bwlq_suf &= ~4;
 	      if (strcasecmp(str, "No_qSuf") == 0)
 		bwlq_suf &= ~8;
+	      if (strcasecmp(str, "No_sSuf") == 0)
+		other_suf &= ~1;
+	      if (strcasecmp(str, "No_ldSuf") == 0)
+		other_suf &= ~2;
 	    }
 	}
 
@@ -1137,7 +1142,26 @@  process_i386_opcode_modifier (FILE *tabl
 	fprintf (stderr,
 		 "%s: %d: W modifier without Word/Dword/Qword operand(s)\n",
 		 filename, lineno);
+      if (!bwlq_suf && !other_suf)
+	fprintf (stderr, "%s: %d: pointless specification of all No_*Suf\n",
+		 filename, lineno);
+    }
+
+  /* Absence of any No_*Suf specification at all is taken to mean all of
+     these attributes to be set, as no template possibly permits for all
+     of them.  */
+  if (opnd && bwlq_suf == 0xf && other_suf == 3)
+    {
+      unsigned int i;
+
+      static_assert (No_lSuf > No_bSuf && No_lSuf < No_ldSuf);
+      static_assert (No_qSuf > No_bSuf && No_qSuf < No_ldSuf);
+      static_assert (No_sSuf > No_bSuf && No_sSuf < No_ldSuf);
+      static_assert (No_wSuf > No_bSuf && No_wSuf < No_ldSuf);
+      for (i = No_bSuf; i <= No_ldSuf; ++i)
+	modifiers[i].value = 1;
     }
+
   output_opcode_modifier (table, modifiers, ARRAY_SIZE (modifiers));
 }