* S390: Allowing a NOP instruction without operands
@ 2009-06-17 16:23 Nick Clifton
2009-06-18 9:38 ` Martin Schwidefsky
0 siblings, 1 reply; 4+ messages in thread
From: Nick Clifton @ 2009-06-17 16:23 UTC (permalink / raw)
To: schwidefsky; +Cc: binutils
[-- Attachment #1: Type: text/plain, Size: 658 bytes --]
Hi Martin,
A Fedora user recently filed a binutils bug report about the NOP
instruction for the s390:
https://bugzilla.redhat.com/show_bug.cgi?id=506417
In it they report that the s390's NOP instruction has to have some
operands specified, which seems rather counter-intuitive given that
it does not do anything.
I have proposed a patch (attached here and also available via that
bugzilla page) to allow the assembler to ignore any missing operands
when a NOP instruction is found. What do you think ? Would this be
OK for the s390 port or is there a reason why the NOP instruction
should take some parameters ?
Cheers
Nick
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tc-s390.c.patch --]
[-- Type: text/x-patch, Size: 1173 bytes --]
Index: gas/config/tc-s390.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-s390.c,v
retrieving revision 1.56
diff -c -3 -p -r1.56 tc-s390.c
*** gas/config/tc-s390.c 12 Aug 2008 23:39:31 -0000 1.56
--- gas/config/tc-s390.c 17 Jun 2009 15:57:06 -0000
*************** md_gather_operands (char *str,
*** 1188,1194 ****
if (ex.X_op == O_illegal)
as_bad (_("illegal operand"));
else if (ex.X_op == O_absent)
! as_bad (_("missing operand"));
else if (ex.X_op == O_register || ex.X_op == O_constant)
{
s390_lit_suffix (&str, &ex, ELF_SUFFIX_NONE);
--- 1188,1200 ----
if (ex.X_op == O_illegal)
as_bad (_("illegal operand"));
else if (ex.X_op == O_absent)
! {
! /* Special case for the NOP instruction. In order to allow
! the (naive) user to just put 'nop' into their assembly
! code without any operands we skip the error generation here. */
! if (strcmp (opcode->name, "nop") != 0)
! as_bad (_("missing operand"));
! }
else if (ex.X_op == O_register || ex.X_op == O_constant)
{
s390_lit_suffix (&str, &ex, ELF_SUFFIX_NONE);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: S390: Allowing a NOP instruction without operands
2009-06-17 16:23 S390: Allowing a NOP instruction without operands Nick Clifton
@ 2009-06-18 9:38 ` Martin Schwidefsky
2009-06-18 10:29 ` Nick Clifton
0 siblings, 1 reply; 4+ messages in thread
From: Martin Schwidefsky @ 2009-06-18 9:38 UTC (permalink / raw)
To: Nick Clifton; +Cc: binutils, Andreas Krebbel
Hi Nick,
On Wed, 17 Jun 2009 17:09:11 +0100
Nick Clifton <nickc@redhat.com> wrote:
> A Fedora user recently filed a binutils bug report about the NOP
> instruction for the s390:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=506417
>
> In it they report that the s390's NOP instruction has to have some
> operands specified, which seems rather counter-intuitive given that
> it does not do anything.
>
> I have proposed a patch (attached here and also available via that
> bugzilla page) to allow the assembler to ignore any missing operands
> when a NOP instruction is found. What do you think ? Would this be
> OK for the s390 port or is there a reason why the NOP instruction
> should take some parameters ?
First of all: yes I think it makes sense to have a nop instruction that
can be used without operands. I don't like the implementation though,
to check for particular opcodes with a strcmp feels wrong. My counter
proposal would be to introduce new variantes for the RR_0R and RX_0RRD
instruction formats. Make all operands optional and replace the strcmp
check with a check against S390_OPERAND_OPTIONAL. And someday I have to
rewrite the whole optional operands logic ..
--
diff -urpN src/gas/config/tc-s390.c src-s390/gas/config/tc-s390.c
--- src/gas/config/tc-s390.c 2009-04-20 12:51:45.000000000 +0200
+++ src-s390/gas/config/tc-s390.c 2009-06-18 10:49:10.000000000 +0200
@@ -1188,7 +1188,24 @@ md_gather_operands (char *str,
if (ex.X_op == O_illegal)
as_bad (_("illegal operand"));
else if (ex.X_op == O_absent)
- as_bad (_("missing operand"));
+ {
+ /* No operands, check if all operands can be skipped. */
+ while (*opindex_ptr != 0 && operand->flags & S390_OPERAND_OPTIONAL)
+ {
+ if (operand->flags & S390_OPERAND_DISP)
+ {
+ /* An optional displacement makes the whole D(X,B)
+ D(L,B) or D(B) block optional. */
+ do {
+ operand = s390_operands + *(++opindex_ptr);
+ } while (!(operand->flags & S390_OPERAND_BASE));
+ }
+ operand = s390_operands + *(++opindex_ptr);
+ }
+ if (opindex_ptr[0] == '\0')
+ break;
+ as_bad (_("missing operand"));
+ }
else if (ex.X_op == O_register || ex.X_op == O_constant)
{
s390_lit_suffix (&str, &ex, ELF_SUFFIX_NONE);
diff -urpN src/opcodes/s390-opc.c src-s390/opcodes/s390-opc.c
--- src/opcodes/s390-opc.c 2008-11-17 15:59:12.000000000 +0100
+++ src-s390/opcodes/s390-opc.c 2009-06-18 10:25:09.000000000 +0200
@@ -48,132 +48,136 @@ const struct s390_operand s390_operands[
{ 4, 8, S390_OPERAND_GPR },
#define R_12 2 /* GPR starting at position 12 */
{ 4, 12, S390_OPERAND_GPR },
-#define R_16 3 /* GPR starting at position 16 */
+#define RO_12 3 /* optional GPR starting at position 12 */
+ { 4, 12, S390_OPERAND_GPR|S390_OPERAND_OPTIONAL },
+#define R_16 4 /* GPR starting at position 16 */
{ 4, 16, S390_OPERAND_GPR },
-#define R_20 4 /* GPR starting at position 20 */
+#define R_20 5 /* GPR starting at position 20 */
{ 4, 20, S390_OPERAND_GPR },
-#define R_24 5 /* GPR starting at position 24 */
+#define R_24 6 /* GPR starting at position 24 */
{ 4, 24, S390_OPERAND_GPR },
-#define R_28 6 /* GPR starting at position 28 */
+#define R_28 7 /* GPR starting at position 28 */
{ 4, 28, S390_OPERAND_GPR },
-#define RO_28 7 /* optional GPR starting at position 28 */
+#define RO_28 8 /* optional GPR starting at position 28 */
{ 4, 28, (S390_OPERAND_GPR | S390_OPERAND_OPTIONAL) },
-#define R_32 8 /* GPR starting at position 32 */
+#define R_32 9 /* GPR starting at position 32 */
{ 4, 32, S390_OPERAND_GPR },
/* Floating point register operands. */
-#define F_8 9 /* FPR starting at position 8 */
+#define F_8 10 /* FPR starting at position 8 */
{ 4, 8, S390_OPERAND_FPR },
-#define F_12 10 /* FPR starting at position 12 */
+#define F_12 11 /* FPR starting at position 12 */
{ 4, 12, S390_OPERAND_FPR },
-#define F_16 11 /* FPR starting at position 16 */
+#define F_16 12 /* FPR starting at position 16 */
{ 4, 16, S390_OPERAND_FPR },
-#define F_20 12 /* FPR starting at position 16 */
+#define F_20 13 /* FPR starting at position 16 */
{ 4, 16, S390_OPERAND_FPR },
-#define F_24 13 /* FPR starting at position 24 */
+#define F_24 14 /* FPR starting at position 24 */
{ 4, 24, S390_OPERAND_FPR },
-#define F_28 14 /* FPR starting at position 28 */
+#define F_28 15 /* FPR starting at position 28 */
{ 4, 28, S390_OPERAND_FPR },
-#define F_32 15 /* FPR starting at position 32 */
+#define F_32 16 /* FPR starting at position 32 */
{ 4, 32, S390_OPERAND_FPR },
/* Access register operands. */
-#define A_8 16 /* Access reg. starting at position 8 */
+#define A_8 17 /* Access reg. starting at position 8 */
{ 4, 8, S390_OPERAND_AR },
-#define A_12 17 /* Access reg. starting at position 12 */
+#define A_12 18 /* Access reg. starting at position 12 */
{ 4, 12, S390_OPERAND_AR },
-#define A_24 18 /* Access reg. starting at position 24 */
+#define A_24 19 /* Access reg. starting at position 24 */
{ 4, 24, S390_OPERAND_AR },
-#define A_28 19 /* Access reg. starting at position 28 */
+#define A_28 20 /* Access reg. starting at position 28 */
{ 4, 28, S390_OPERAND_AR },
/* Control register operands. */
-#define C_8 20 /* Control reg. starting at position 8 */
+#define C_8 21 /* Control reg. starting at position 8 */
{ 4, 8, S390_OPERAND_CR },
-#define C_12 21 /* Control reg. starting at position 12 */
+#define C_12 22 /* Control reg. starting at position 12 */
{ 4, 12, S390_OPERAND_CR },
/* Base register operands. */
-#define B_16 22 /* Base register starting at position 16 */
+#define B_16 23 /* Base register starting at position 16 */
{ 4, 16, S390_OPERAND_BASE|S390_OPERAND_GPR },
-#define B_32 23 /* Base register starting at position 32 */
+#define B_32 24 /* Base register starting at position 32 */
{ 4, 32, S390_OPERAND_BASE|S390_OPERAND_GPR },
-#define X_12 24 /* Index register starting at position 12 */
+#define X_12 25 /* Index register starting at position 12 */
{ 4, 12, S390_OPERAND_INDEX|S390_OPERAND_GPR },
/* Address displacement operands. */
-#define D_20 25 /* Displacement starting at position 20 */
+#define D_20 26 /* Displacement starting at position 20 */
{ 12, 20, S390_OPERAND_DISP },
-#define D_36 26 /* Displacement starting at position 36 */
+#define DO_20 27 /* optional Displ. starting at position 20 */
+ { 12, 20, S390_OPERAND_DISP|S390_OPERAND_OPTIONAL },
+#define D_36 28 /* Displacement starting at position 36 */
{ 12, 36, S390_OPERAND_DISP },
-#define D20_20 27 /* 20 bit displacement starting at 20 */
+#define D20_20 29 /* 20 bit displacement starting at 20 */
{ 20, 20, S390_OPERAND_DISP|S390_OPERAND_SIGNED },
/* Length operands. */
-#define L4_8 28 /* 4 bit length starting at position 8 */
+#define L4_8 30 /* 4 bit length starting at position 8 */
{ 4, 8, S390_OPERAND_LENGTH },
-#define L4_12 29 /* 4 bit length starting at position 12 */
+#define L4_12 31 /* 4 bit length starting at position 12 */
{ 4, 12, S390_OPERAND_LENGTH },
-#define L8_8 30 /* 8 bit length starting at position 8 */
+#define L8_8 32 /* 8 bit length starting at position 8 */
{ 8, 8, S390_OPERAND_LENGTH },
/* Signed immediate operands. */
-#define I8_8 31 /* 8 bit signed value starting at 8 */
+#define I8_8 33 /* 8 bit signed value starting at 8 */
{ 8, 8, S390_OPERAND_SIGNED },
-#define I8_32 32 /* 8 bit signed value starting at 32 */
+#define I8_32 34 /* 8 bit signed value starting at 32 */
{ 8, 32, S390_OPERAND_SIGNED },
-#define I16_16 33 /* 16 bit signed value starting at 16 */
+#define I16_16 35 /* 16 bit signed value starting at 16 */
{ 16, 16, S390_OPERAND_SIGNED },
-#define I16_32 34 /* 16 bit signed value starting at 32 */
+#define I16_32 36 /* 16 bit signed value starting at 32 */
{ 16, 32, S390_OPERAND_SIGNED },
-#define I32_16 35 /* 32 bit signed value starting at 16 */
+#define I32_16 37 /* 32 bit signed value starting at 16 */
{ 32, 16, S390_OPERAND_SIGNED },
/* Unsigned immediate operands. */
-#define U4_8 36 /* 4 bit unsigned value starting at 8 */
+#define U4_8 38 /* 4 bit unsigned value starting at 8 */
{ 4, 8, 0 },
-#define U4_12 37 /* 4 bit unsigned value starting at 12 */
+#define U4_12 39 /* 4 bit unsigned value starting at 12 */
{ 4, 12, 0 },
-#define U4_16 38 /* 4 bit unsigned value starting at 16 */
+#define U4_16 40 /* 4 bit unsigned value starting at 16 */
{ 4, 16, 0 },
-#define U4_20 39 /* 4 bit unsigned value starting at 20 */
+#define U4_20 41 /* 4 bit unsigned value starting at 20 */
{ 4, 20, 0 },
-#define U4_32 40 /* 4 bit unsigned value starting at 32 */
+#define U4_32 42 /* 4 bit unsigned value starting at 32 */
{ 4, 32, 0 },
-#define U8_8 41 /* 8 bit unsigned value starting at 8 */
+#define U8_8 43 /* 8 bit unsigned value starting at 8 */
{ 8, 8, 0 },
-#define U8_16 42 /* 8 bit unsigned value starting at 16 */
+#define U8_16 44 /* 8 bit unsigned value starting at 16 */
{ 8, 16, 0 },
-#define U8_24 43 /* 8 bit unsigned value starting at 24 */
+#define U8_24 45 /* 8 bit unsigned value starting at 24 */
{ 8, 24, 0 },
-#define U8_32 44 /* 8 bit unsigned value starting at 32 */
+#define U8_32 46 /* 8 bit unsigned value starting at 32 */
{ 8, 32, 0 },
-#define U16_16 45 /* 16 bit unsigned value starting at 16 */
+#define U16_16 47 /* 16 bit unsigned value starting at 16 */
{ 16, 16, 0 },
-#define U16_32 46 /* 16 bit unsigned value starting at 32 */
+#define U16_32 48 /* 16 bit unsigned value starting at 32 */
{ 16, 32, 0 },
-#define U32_16 47 /* 32 bit unsigned value starting at 16 */
+#define U32_16 49 /* 32 bit unsigned value starting at 16 */
{ 32, 16, 0 },
/* PC-relative address operands. */
-#define J16_16 48 /* PC relative jump offset at 16 */
+#define J16_16 50 /* PC relative jump offset at 16 */
{ 16, 16, S390_OPERAND_PCREL },
-#define J32_16 49 /* PC relative long offset at 16 */
+#define J32_16 51 /* PC relative long offset at 16 */
{ 32, 16, S390_OPERAND_PCREL },
/* Conditional mask operands. */
-#define M_16 50 /* 4 bit optional mask starting at 16 */
+#define M_16 52 /* 4 bit optional mask starting at 16 */
{ 4, 16, S390_OPERAND_OPTIONAL },
};
@@ -278,6 +282,7 @@ const struct s390_operand s390_operands[
#define INSTR_RRF_U0RR 4, { R_24,R_28,U4_16,0,0,0 } /* e.g. clrt */
#define INSTR_RRF_00RR 4, { R_24,R_28,0,0,0,0 } /* e.g. clrtne */
#define INSTR_RR_0R 2, { R_12, 0,0,0,0,0 } /* e.g. br */
+#define INSTR_RR_0R_OPT 2, { RO_12, 0,0,0,0,0 } /* e.g. nopr */
#define INSTR_RR_FF 2, { F_8,F_12,0,0,0,0 } /* e.g. adr */
#define INSTR_RR_R0 2, { R_8, 0,0,0,0,0 } /* e.g. spm */
#define INSTR_RR_RR 2, { R_8,R_12,0,0,0,0 } /* e.g. lr */
@@ -308,6 +313,7 @@ const struct s390_operand s390_operands[
#define INSTR_RXY_FRRD 6, { F_8,D20_20,X_12,B_16,0,0 } /* e.g. ley */
#define INSTR_RXY_URRD 6, { U4_8,D20_20,X_12,B_16,0,0 } /* e.g. pfd */
#define INSTR_RX_0RRD 4, { D_20,X_12,B_16,0,0,0 } /* e.g. be */
+#define INSTR_RX_0RRD_OPT 4, { DO_20,X_12,B_16,0,0,0 } /* e.g. nop */
#define INSTR_RX_FRRD 4, { F_8,D_20,X_12,B_16,0,0 } /* e.g. ae */
#define INSTR_RX_RRRD 4, { R_8,D_20,X_12,B_16,0,0 } /* e.g. l */
#define INSTR_RX_URRD 4, { U4_8,D_20,X_12,B_16,0,0 } /* e.g. bc */
@@ -382,6 +388,7 @@ const struct s390_operand s390_operands[
#define MASK_RRF_U0RR { 0xff, 0xff, 0x0f, 0x00, 0x00, 0x00 }
#define MASK_RRF_00RR { 0xff, 0xff, 0xff, 0x00, 0x00, 0x00 }
#define MASK_RR_0R { 0xff, 0xf0, 0x00, 0x00, 0x00, 0x00 }
+#define MASK_RR_0R_OPT { 0xff, 0xf0, 0x00, 0x00, 0x00, 0x00 }
#define MASK_RR_FF { 0xff, 0x00, 0x00, 0x00, 0x00, 0x00 }
#define MASK_RR_R0 { 0xff, 0x0f, 0x00, 0x00, 0x00, 0x00 }
#define MASK_RR_RR { 0xff, 0x00, 0x00, 0x00, 0x00, 0x00 }
@@ -412,6 +419,7 @@ const struct s390_operand s390_operands[
#define MASK_RXY_FRRD { 0xff, 0x00, 0x00, 0x00, 0x00, 0xff }
#define MASK_RXY_URRD { 0xff, 0x00, 0x00, 0x00, 0x00, 0xff }
#define MASK_RX_0RRD { 0xff, 0xf0, 0x00, 0x00, 0x00, 0x00 }
+#define MASK_RX_0RRD_OPT { 0xff, 0xf0, 0x00, 0x00, 0x00, 0x00 }
#define MASK_RX_FRRD { 0xff, 0x00, 0x00, 0x00, 0x00, 0x00 }
#define MASK_RX_RRRD { 0xff, 0x00, 0x00, 0x00, 0x00, 0x00 }
#define MASK_RX_URRD { 0xff, 0x00, 0x00, 0x00, 0x00, 0x00 }
diff -urpN src/opcodes/s390-opc.txt src-s390/opcodes/s390-opc.txt
--- src/opcodes/s390-opc.txt 2008-11-19 11:27:07.000000000 +0100
+++ src-s390/opcodes/s390-opc.txt 2009-06-18 09:51:16.000000000 +0200
@@ -260,10 +260,10 @@ b252 msr RRE_RR "multiply single" g5 esa
71 ms RX_RRRD "multiply single" g5 esa,zarch
a700 tmh RI_RU "test under mask high" g5 esa,zarch
a701 tml RI_RU "test under mask low" g5 esa,zarch
-0700 nopr RR_0R "no operation" g5 esa,zarch
+0700 nopr RR_0R_OPT "no operation" g5 esa,zarch
0700 b*8r RR_0R "conditional branch" g5 esa,zarch
07f0 br RR_0R "unconditional branch" g5 esa,zarch
-4700 nop RX_0RRD "no operation" g5 esa,zarch
+4700 nop RX_0RRD_OPT "no operation" g5 esa,zarch
4700 b*8 RX_0RRD "conditional branch" g5 esa,zarch
47f0 b RX_0RRD "unconditional branch" g5 esa,zarch
a704 j*8 RI_0P "conditional jump" g5 esa,zarch
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: S390: Allowing a NOP instruction without operands
2009-06-18 9:38 ` Martin Schwidefsky
@ 2009-06-18 10:29 ` Nick Clifton
2009-06-19 13:34 ` Martin Schwidefsky
0 siblings, 1 reply; 4+ messages in thread
From: Nick Clifton @ 2009-06-18 10:29 UTC (permalink / raw)
To: Martin Schwidefsky; +Cc: binutils, Andreas Krebbel
Hi Martin,
> First of all: yes I think it makes sense to have a nop instruction that
> can be used without operands. I don't like the implementation though,
> to check for particular opcodes with a strcmp feels wrong.
I thought that you might say that. :-)
> My counter
> proposal would be to introduce new variantes for the RR_0R and RX_0RRD
> instruction formats. Make all operands optional and replace the strcmp
> check with a check against S390_OPERAND_OPTIONAL.
Fine by me. I just went with the simplest solution. (Plus I have never
looked at the s390 opcode generation system before and I was worried
that if I tried to introduce a new nop pattern I would break something).
Please could you go ahead and apply your patch ?
Cheers
Nick
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: S390: Allowing a NOP instruction without operands
2009-06-18 10:29 ` Nick Clifton
@ 2009-06-19 13:34 ` Martin Schwidefsky
0 siblings, 0 replies; 4+ messages in thread
From: Martin Schwidefsky @ 2009-06-19 13:34 UTC (permalink / raw)
To: Nick Clifton; +Cc: binutils, Andreas Krebbel
On Thu, 18 Jun 2009 11:10:42 +0100
Nick Clifton <nickc@redhat.com> wrote:
> Hi Martin,
>
> > First of all: yes I think it makes sense to have a nop instruction that
> > can be used without operands. I don't like the implementation though,
> > to check for particular opcodes with a strcmp feels wrong.
>
> I thought that you might say that. :-)
>
> > My counter
> > proposal would be to introduce new variantes for the RR_0R and RX_0RRD
> > instruction formats. Make all operands optional and replace the strcmp
> > check with a check against S390_OPERAND_OPTIONAL.
>
> Fine by me. I just went with the simplest solution. (Plus I have never
> looked at the s390 opcode generation system before and I was worried
> that if I tried to introduce a new nop pattern I would break something).
>
> Please could you go ahead and apply your patch ?
Done.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-06-19 10:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-17 16:23 S390: Allowing a NOP instruction without operands Nick Clifton
2009-06-18 9:38 ` Martin Schwidefsky
2009-06-18 10:29 ` Nick Clifton
2009-06-19 13:34 ` Martin Schwidefsky
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).