OpenCores
Issue List
l.maci does not assemble and disassemble consistently #108
Closed jeremybennett opened this issue over 14 years ago
jeremybennett commented over 14 years ago
<p> The opcode <tt>l.maci</tt> is executed by the OR1200 assuming the constant field is in bits [15:0]. However the assembler assembles the code as though the constant field were in bits [25:21] and [10:0], as described in the architecture manual. </p> <p> The disassembler looks for the register argument (rA) in bits [15:11]. This is the location of rB specified in the architecture manual, which then describes the opcode in terms of rA, which is not specified. </p> <p> At present the l.maci opcode does not appear to execute correctly on the OR1200. (see <a href="http://opencores.org/bug,view,1786">Bug 1786</a>). However Or1ksim should execute it correctly, assuming the same instruction format accepted by the OR1200 Verilog. </p> <p> The assembler/disassembler need updating (binutils). This will affect other parts of the tool chain, which use the same libraries (GCC and GDB). </p> <p> Jeremy </p> <p> -- <br /> Tel: +44 (1590) 610184<br /> Cell: +44 (7970) 676050<br /> SkypeID: jeremybennett<br /> Email: <a href="mailto:jeremy.bennett@embecosm.com">jeremy.bennett@embecosm.com</a><br /> Web: <a href="http://www.embecosm.com">www.embecosm.com</a> </p>
julius commented over 14 years ago

Binutils has the following for this instruction:

{ "l.maci", "rA,I", "01 0x3 IIIII AAAAA ---- -III IIII IIII", EF(l_mac), 0, it_mac },

The architecture spec, then, is wrong in indicating A's register address field as reserved, and B's being used.

The supplementary spec (http://opencores.org/ocsvn/openrisc/openrisc/trunk/docs/openrisc1200v2_supplementary_prm.odt ) and, I think or1ksim, now has immediate in 15:0 and register number in A field is used.

Binutils opcode table for OR32 still has immediate as bits 25:21 and 10:0.

Option 1: Update supplementary manual to indicate immediate value in 25:21 and 10:0, and fix OR1200 and or1ksim to use immediate in those locations. This would not require a change to binutils opcode table.

Option 2: Update binutils opcode table indicating immediate is now in bits 15:0, bringing it in line with supplementary manual, or1ksim and or1200.

jeremybennett commented over 14 years ago
<p> I recommend Option 2. Since the implementation is out of line with the architecture manual, something has to change. The current implementation is anomalous. I believe it is the only instruction with a 16-bit operand not held in [15:0]. Bringing it in line with other instructions allows potential reuse of decode logic. </p> <p> I would be fairly certain that no one has ever used this opcode. It has only recently been corrected in Or1ksim, and I believe is yet to be implemented in the RTL? </p> <p> Jeremy </p>
julius commented over 14 years ago

Actually, it looks like the OR1200 does fish the immediate from bits 25:21 and 10:0, as can be seen in or1200_ctrl.v:

ifdef OR1200_MAC_IMPLEMENTED OR1200_OR32_MACI: id_simm = {{16{id_insn[25]}}, id_insn[25:21], id_insn[10:0]};endif

The other instructions to use these immediate fields is the store data instructions (l.sw, l.sh, l.sb), so this isn't an unusual way to pack in an immediate.

Maybe we should stick to the original implementation, only updating the supplementary manual to indicate the A field for register address is used. This would mean no change to the OR1200 RTL, and a very minimal alteration of the instruction in the supplementary manual, as well, as, I guess, bringing or1ksim in line with this.

jeremybennett commented over 13 years ago
<p> Transferred to OpenRISC bugzilla (<a href="http://bugzilla.opencores.org/show_bug.cgi?id=18">Bug 18</a>). </p> <p> Marking closed in this bugtracker. </p>
jeremybennett was assigned over 13 years ago
jeremybennett closed this over 13 years ago

Assignee
jeremybennett
Labels
Bug