This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
Re: [PATCH] more ARC opcodes cleanups.
- From: Ramana Radhakrishnan <ramana dot radhakrishnan at codito dot com>
- To: Nick Clifton <nickc at redhat dot com>
- Cc: binutils at sources dot redhat dot com
- Date: Thu, 03 Mar 2005 21:01:58 +0530
- Subject: Re: [PATCH] more ARC opcodes cleanups.
- References: <422705D4.2070005@codito.com> <4227273A.8090503@redhat.com>
Hi Nick,
<snip>
Sorry no.
There are three problems:
1. You are using a set of #define's for the arithmetic classes, which
is better than the straight constants that were being used before, but
for something like this an enum is the correct way to go.
Ok. Done.
2. When you add comments, please make sure that they follow the
guidelines for comment format. In this case they should be treated as
full sentences and end with a full stop followed by two spaces before
the */ characters.
Done.
3. You appear to be adding in extra blank lines for no really good
reason.
Done.
Thanks for reviewing the patch. Ok to commit now ?
cheers
Ramana
--
Ramana Radhakrishnan
GNU Tools
codito ergo sum (www.codito.com)
Index: arc-dis.c
===================================================================
RCS file: /cvs/src/src/opcodes/arc-dis.c,v
retrieving revision 1.8
diff -c -3 -p -r1.8 arc-dis.c
*** arc-dis.c 25 May 2002 12:55:19 -0000 1.8
--- arc-dis.c 3 Mar 2005 15:19:01 -0000
***************
*** 34,39 ****
--- 34,62 ----
#define dbg (0)
#endif
+
+ /* Classification of the opcodes for the decoder to print
+ the instructions. */
+
+ typedef enum {
+ CLASS_A4_ARITH,
+ CLASS_A4_OP3_GENERAL,
+ CLASS_A4_FLAG,
+ /* All branches other than JC. */
+ CLASS_A4_BRANCH,
+ CLASS_A4_JC ,
+ /* All loads other than immediate
+ indexed loads. */
+ CLASS_A4_LD0,
+ CLASS_A4_LD1,
+ CLASS_A4_ST,
+ CLASS_A4_SR,
+ /* All single operand instructions. */
+ CLASS_A4_OP3_SUBOPC3F,
+ CLASS_A4_LR
+ } a4_decoding_class;
+
+
#define BIT(word,n) ((word) & (1 << n))
#define BITS(word,s,e) (((word) << (31 - e)) >> (s + (31 - e)))
#define OPCODE(word) (BITS ((word), 27, 31))
***************
*** 41,46 ****
--- 64,70 ----
#define FIELDB(word) (BITS ((word), 15, 20))
#define FIELDC(word) (BITS ((word), 9, 14))
+
/* FIELD D is signed in all of its uses, so we make sure argument is
treated as signed for bit shifting purposes: */
#define FIELDD(word) (BITS (((signed int)word), 0, 8))
*************** dsmOneArcInst (addr, state)
*** 531,537 ****
struct arcDisState * state;
{
int condCodeIsPartOfName = 0;
! int decodingClass;
const char * instrName;
int repeatsOp = 0;
int fieldAisReg = 1;
--- 555,561 ----
struct arcDisState * state;
{
int condCodeIsPartOfName = 0;
! a4_decoding_class decodingClass;
const char * instrName;
int repeatsOp = 0;
int fieldAisReg = 1;
*************** dsmOneArcInst (addr, state)
*** 572,578 ****
state->_opcode = OPCODE (state->words[0]);
instrName = 0;
! decodingClass = 0; /* default! */
repeatsOp = 0;
condCodeIsPartOfName=0;
state->commNum = 0;
--- 596,602 ----
state->_opcode = OPCODE (state->words[0]);
instrName = 0;
! decodingClass = CLASS_A4_ARITH; /* default! */
repeatsOp = 0;
condCodeIsPartOfName=0;
state->commNum = 0;
*************** dsmOneArcInst (addr, state)
*** 606,619 ****
state->flow = invalid_instr;
break;
}
! decodingClass = 5;
break;
case op_LD1:
if (BIT (state->words[0],13))
{
instrName = "lr";
! decodingClass = 10;
}
else
{
--- 630,643 ----
state->flow = invalid_instr;
break;
}
! decodingClass = CLASS_A4_LD0;
break;
case op_LD1:
if (BIT (state->words[0],13))
{
instrName = "lr";
! decodingClass = CLASS_A4_LR;
}
else
{
*************** dsmOneArcInst (addr, state)
*** 636,642 ****
state->flow = invalid_instr;
break;
}
! decodingClass = 6;
}
break;
--- 660,666 ----
state->flow = invalid_instr;
break;
}
! decodingClass = CLASS_A4_LD1;
}
break;
*************** dsmOneArcInst (addr, state)
*** 644,650 ****
if (BIT (state->words[0],25))
{
instrName = "sr";
! decodingClass = 8;
}
else
{
--- 668,674 ----
if (BIT (state->words[0],25))
{
instrName = "sr";
! decodingClass = CLASS_A4_SR;
}
else
{
*************** dsmOneArcInst (addr, state)
*** 664,680 ****
state->flow = invalid_instr;
break;
}
! decodingClass = 7;
}
break;
case op_3:
! decodingClass = 1; /* default for opcode 3... */
switch (FIELDC (state->words[0]))
{
case 0:
instrName = "flag";
! decodingClass = 2;
break;
case 1:
instrName = "asr";
--- 688,704 ----
state->flow = invalid_instr;
break;
}
! decodingClass = CLASS_A4_ST;
}
break;
case op_3:
! decodingClass = CLASS_A4_OP3_GENERAL; /* default for opcode 3... */
switch (FIELDC (state->words[0]))
{
case 0:
instrName = "flag";
! decodingClass = CLASS_A4_FLAG;
break;
case 1:
instrName = "asr";
*************** dsmOneArcInst (addr, state)
*** 702,708 ****
break;
case 0x3f:
{
! decodingClass = 9;
switch( FIELDD (state->words[0]) )
{
case 0:
--- 726,732 ----
break;
case 0x3f:
{
! decodingClass = CLASS_A4_OP3_SUBOPC3F;
switch( FIELDD (state->words[0]) )
{
case 0:
*************** dsmOneArcInst (addr, state)
*** 763,769 ****
}
}
condCodeIsPartOfName = 1;
! decodingClass = ((state->_opcode == op_JC) ? 4 : 3);
state->isBranch = 1;
break;
--- 787,793 ----
}
}
condCodeIsPartOfName = 1;
! decodingClass = ((state->_opcode == op_JC) ? CLASS_A4_JC : CLASS_A4_BRANCH );
state->isBranch = 1;
break;
*************** dsmOneArcInst (addr, state)
*** 771,777 ****
case op_ADC:
case op_AND:
repeatsOp = (FIELDC (state->words[0]) == FIELDB (state->words[0]));
- decodingClass = 0;
switch (state->_opcode)
{
--- 795,800 ----
*************** dsmOneArcInst (addr, state)
*** 801,807 ****
{
/* nop encoded as xor -1, -1, -1 */
instrName = "nop";
! decodingClass = 9;
}
else
instrName = "xor";
--- 824,830 ----
{
/* nop encoded as xor -1, -1, -1 */
instrName = "nop";
! decodingClass = CLASS_A4_OP3_SUBOPC3F;
}
else
instrName = "xor";
*************** dsmOneArcInst (addr, state)
*** 828,834 ****
switch (decodingClass)
{
! case 0:
CHECK_FIELD_A ();
CHECK_FIELD_B ();
if (!repeatsOp)
--- 851,857 ----
switch (decodingClass)
{
! case CLASS_A4_ARITH:
CHECK_FIELD_A ();
CHECK_FIELD_B ();
if (!repeatsOp)
*************** dsmOneArcInst (addr, state)
*** 857,863 ****
write_comments ();
break;
! case 1:
CHECK_FIELD_A ();
CHECK_FIELD_B ();
CHECK_FLAG_COND_NULLIFY ();
--- 880,886 ----
write_comments ();
break;
! case CLASS_A4_OP3_GENERAL:
CHECK_FIELD_A ();
CHECK_FIELD_B ();
CHECK_FLAG_COND_NULLIFY ();
*************** dsmOneArcInst (addr, state)
*** 879,885 ****
write_comments ();
break;
! case 2:
CHECK_FIELD_B ();
CHECK_FLAG_COND_NULLIFY ();
flag = 0; /* this is the FLAG instruction -- it's redundant */
--- 902,908 ----
write_comments ();
break;
! case CLASS_A4_FLAG:
CHECK_FIELD_B ();
CHECK_FLAG_COND_NULLIFY ();
flag = 0; /* this is the FLAG instruction -- it's redundant */
*************** dsmOneArcInst (addr, state)
*** 890,896 ****
write_comments ();
break;
! case 3:
fieldA = BITS (state->words[0],7,26) << 2;
fieldA = (fieldA << 10) >> 10; /* make it signed */
fieldA += addr + 4;
--- 913,919 ----
write_comments ();
break;
! case CLASS_A4_BRANCH:
fieldA = BITS (state->words[0],7,26) << 2;
fieldA = (fieldA << 10) >> 10; /* make it signed */
fieldA += addr + 4;
*************** dsmOneArcInst (addr, state)
*** 915,921 ****
write_comments ();
break;
! case 4:
/* For op_JC -- jump to address specified.
Also covers jump and link--bit 9 of the instr. word
selects whether linked, thus "is_linked" is set above. */
--- 938,944 ----
write_comments ();
break;
! case CLASS_A4_JC:
/* For op_JC -- jump to address specified.
Also covers jump and link--bit 9 of the instr. word
selects whether linked, thus "is_linked" is set above. */
*************** dsmOneArcInst (addr, state)
*** 961,967 ****
write_comments ();
break;
! case 5:
/* LD instruction.
B and C can be regs, or one (both?) can be limm. */
CHECK_FIELD_A ();
--- 984,990 ----
write_comments ();
break;
! case CLASS_A4_LD0:
/* LD instruction.
B and C can be regs, or one (both?) can be limm. */
CHECK_FIELD_A ();
*************** dsmOneArcInst (addr, state)
*** 999,1005 ****
write_comments ();
break;
! case 6:
/* LD instruction. */
CHECK_FIELD_B ();
CHECK_FIELD_A ();
--- 1022,1028 ----
write_comments ();
break;
! case CLASS_A4_LD1:
/* LD instruction. */
CHECK_FIELD_B ();
CHECK_FIELD_A ();
*************** dsmOneArcInst (addr, state)
*** 1045,1051 ****
write_comments ();
break;
! case 7:
/* ST instruction. */
CHECK_FIELD_B();
CHECK_FIELD_C();
--- 1068,1074 ----
write_comments ();
break;
! case CLASS_A4_ST:
/* ST instruction. */
CHECK_FIELD_B();
CHECK_FIELD_C();
*************** dsmOneArcInst (addr, state)
*** 1090,1096 ****
fieldC, fieldB, fieldA);
write_comments2(fieldA);
break;
! case 8:
/* SR instruction */
CHECK_FIELD_B();
CHECK_FIELD_C();
--- 1113,1120 ----
fieldC, fieldB, fieldA);
write_comments2(fieldA);
break;
!
! case CLASS_A4_SR:
/* SR instruction */
CHECK_FIELD_B();
CHECK_FIELD_C();
*************** dsmOneArcInst (addr, state)
*** 1105,1116 ****
write_comments();
break;
! case 9:
write_instr_name();
state->operandBuffer[0] = '\0';
break;
! case 10:
/* LR instruction */
CHECK_FIELD_A();
CHECK_FIELD_B();
--- 1129,1140 ----
write_comments();
break;
! case CLASS_A4_OP3_SUBOPC3F:
write_instr_name();
state->operandBuffer[0] = '\0';
break;
! case CLASS_A4_LR:
/* LR instruction */
CHECK_FIELD_A();
CHECK_FIELD_B();
*************** dsmOneArcInst (addr, state)
*** 1125,1135 ****
write_comments();
break;
- case 11:
- CHECK_COND();
- write_instr_name();
- state->operandBuffer[0] = '\0';
- break;
default:
mwerror (state, "Bad decoding class in ARC disassembler");
--- 1149,1154 ----
2005-03-03 Ramana Radhakrishnan <ramana.radhakrishnan@codito.com>
* opcodes/arc-dis.c:Add enum a4_decoding_class.
(dsmOneArcInst): Use the enum values for the decoding class
Remove redundant case in the switch for decodingClass value 11