I am testing the software in the svn path:"/openrisc/trunk/orpsocv2/sw/tests/or1200/sim/or1200-maci.S". I found there is a bug in the verilog code.
The behaviors of l.maci, l.mac and l.msb in the recent verilog code are not the same as that in the or1ksim. In the or1ksim, the 64bit multiply value is truncated to 32bit and then sign extended to 64bit, before add/sub to {MACHI,MACLO}. But, in the verilog code, the 64bit multiply value is directly add/sub to {MACHI,MACLO}.
The document is: l.maci should be truncated and sign extended, but l.mac and l.msb are only truncated.
Which is right?

Regarding the verilog, I think it's correct - the sign-extended immediate is generated in or1200_ctrl.v and passed through the muxes to the or1200_mult_mac unit, where it is multiplied with the contents of rA as it should be.

I'm not sure what you mean by "l.maci should be truncated and sign-extended", which document are you talking about here? As far as I can see, it's a 16-bit immediate and there's no truncation required prior to its use, just sign-extension to 32-bits for a 32-bit*32-bit multiply, resulting in a 64-bit result.

Regarding what or1ksim does, what ends up in the complex execution model file (generated when compiling, look in or1ksim-build-dir/cpu/or32/execgen.c) all looks perfectly fine to me. Please post what you think is incorrect.

Thanks,

Julius

Thank you, Julius.

I will describe the problem clearly.

1: About truncation, the document "openrisc_arch.pdf" says that --
~~~~~
Format: l.mac rA,rB
Description:
The contents of general-purpose register rA and the contents of
general-purpose register rB are multiplied, and the result is truncated
to 32 bits and added to the special-purpose registers MACHI and MACLO.
All operands are treated as signed integers.
32-bit Implementation:
temp[31:0] MACHI[31:0]MACLO[31:0] ~~~~~

It is obvious that only low 32bit result of rA[31:0] * rB[31:0] is
added to MACHI[31:0]MACLO[31:0].
But in the or1200_mult_mac.v ---
~~~~~
//
// Implementation of MAC
//
always @(`OR1200_RST_EVENT rst or posedge clk)
if (rst == `OR1200_RST_VALUE)
mac_r = 64'h0000_0000_0000_0000;
`ifdef OR1200_MAC_SPR_WE
else if (spr_maclo_we)
mac_r[31:0] = spr_dat_i;
else if (spr_machi_we)
mac_r[63:32] = spr_dat_i;
`endif
else if (mac_op_r3 == `OR1200_MACOP_MAC)
mac_r = mac_r + mul_prod_r;
else if (mac_op_r3 == `OR1200_MACOP_MSB)
mac_r = mac_r - mul_prod_r;
else if (macrc_op && !ex_freeze)
mac_r = 64'h0000_0000_0000_0000;
~~~~~
All 64bit mul_prod_r is added to mac_r. So the result is not identical.

2: About sign-extended, the document "openrisc1200_supplementary_prm.pdf"
says that --
~~~~~
Format: l.maci rA,I
Description
The immediate value and the contents of general-purpose register rA are
multiplied, and the result is truncated to 32 bits, then sign extended
and added to the special- purpose registers MACHI and MACLO. All operands
are treated as signed integers.

32-bit Implementation
temp1[31:0] temp1[63:32] temp2[31:0] temp1[63:0] MACLO[31:0] MACHI[31:0] ~~~~~~
What is called sign-extended is exts(temp2[31:0]).

3: I use the or1ksim-0.5.1 installed by "or32-elf-1.0rc1-x86.tar.bz2". Run
the test software or1200-maci.S (in the svn path: /openrisc/trunk/orpsocv2/
sw/tests/or1200/sim/ ). Part of the program is ---
~~~~~~
/* Jeremy stuff. A l.maci which should overflow and be truncated. */
l.movhi r4,hi(0x7fffffff)
l.ori r4,r4,lo(0x7fffffff)

l.mtspr r0,r0,SPR_MACHI
l.mtspr r0,r0,SPR_MACLO
l.maci r4,0x7fff

l.mfspr r3,r0,SPR_MACHI
l.nop 0x2
l.nop
l.mfspr r3,r0,SPR_MACLO
l.nop 0x2
l.nop
~~~~~~
Because of truncation,
The or1ksim result is {SPR_MACHI,SPR_MACLO} = 0x7fff8001.
The verilog result is {SPR_MACHI,SPR_MACLO} = 0x3fff_7fff8001.
Change the r4 to 0x3fffffff, because of sign-extended,
The or1ksim result is {SPR_MACHI,SPR_MACLO} = 0xffffffff_bfff8001.
The verilog result is {SPR_MACHI,SPR_MACLO} = 0x1fff_bfff8001.


The same thing is found in l.msb. So the or1ksim and the document are the
same, but the verilog not. You say "Regarding what or1ksim does", but it is
really according to the document. ("openrisc_arch.pdf", find in the svn path:
/openrisc/trunk/docs/)

Thanks.

Sorry, the edit has some problem.

the document "openrisc_arch.pdf":
32-bit Implementation:
temp[31:0] = rA[31:0] * rB[31:0]
MACHI[31:0]MACLO[31:0] = temp[31:0] + MACHI[31:0]MACLO[31:0]

the document "openrisc1200_supplementary_prm.pdf":
32-bit Implementation
temp1[31:0] = MACLO[31:0]
temp1[63:32] = MACHI[31:0]
temp2[31:0] = rA[31:0] * exts(Immediate)
temp1[63:0] = temp1[63:0] + exts(temp2[31:0])
MACLO[31:0] = temp1[31:0]
MACHI[31:0] = temp1[63:32]

It seems what you've found is a inconsistency between revisions of the openrisc 1000 specification
and between or1ksim and the or1200 rtl.

The latest revision of the openrisc specification:
http://opencores.org/websvn,filedetails?repname=openrisc&path=%2Fopenrisc%2Ftrunk%2Fdocs%2Fopenrisc_arch_draft.odt
have been updated in regards of the l.mac and l.maci instructions:

---
l.mac rA,rB
Description:
The contents of general-purpose register rA and the contents of general-purpose register rB are multiplied, and the 64 bit result is added to the special-purpose registers MACHI and MACLO. All operands are treated as signed integers.
32-bit Implementation:
MACHI[31:0]MACLO[31:0] � MACHI[31:0]MACLO[31:0] +
rA[31:0] * rB[31:0]
64-bit Implementation:

MACHI[31:0]MACLO[31:0] � MACHI[31:0]MACLO[31:0] +
rA[63:0] * rB[63:0]
---
l.maci rA,I
Description:
The immediate value and the contents of general-purpose register rA are multiplied, and the 64 bit result is added to the special-purpose registers MACHI and MACLO. All operands are treated as signed integers.
32-bit Implementation:
MACHI[31:0]MACLO[31:0] � MACHI[31:0]MACLO[31:0] +
rA[31:0] * exts(Immediate)
64-bit Implementation:
MACHI[31:0]MACLO[31:0] � MACHI[31:0]MACLO[31:0] +
rA[63:0] * exts(Immediate)
---

As it appears, this is how the RTL works, but or1ksim works according to the older spec.

This is clearly a mess that needs sorting out, thanks for noticing this.

Stefan

Obviously this definition of the MAC instructions has changed during the architecture spec audit, and I think from a DSP perspective (although, not sure) we _don't_ want to truncate the multiply before addition. I'm not positive on this, though, and if someone with more knowledge on DSP would like to comment and indicate which is more desirable, that would be appreciated. But at present, all implementations (or1ksim in the repository, and the Verilog) do _not_ truncate the multiplication result before addition to the MACHI,MACLO registers.

or1ksim does the following, in cpu/or32/execgen.c (generated during compile) for l.mac (i've removed the original comments from the source, as they are misleading, and added my own - perhaps we need do this in the checked-in version ):

{ /* "l_mac" */
uorreg_t lo, hi;
LONGEST l;
orreg_t x, y, t;
/* setting up operands */
lo = cpu_state.sprs[SPR_MACLO];
hi = cpu_state.sprs[SPR_MACHI];
x = PARAM0;
y = PARAM1;
l = (ULONGEST)lo | ((LONGEST)hi /* the multiply */
t = x * y;
/* the addition - no truncation */
l += (LONGEST) t;
/* split up the result for storage back in MACHI and MACLO registers */
lo = ((ULONGEST)l) & 0xFFFFFFFF;
hi = ((LONGEST)l) >> 32;
cpu_state.sprs[SPR_MACLO] = lo;
cpu_state.sprs[SPR_MACHI] = hi;
}

I must admit guilt in changing the specification here. While I did check the RTL behaviour in OR1200, it did not occur to me to check or1ksim for the described behaviour, which has a few strange implications.

First, the truncation discards the more significant bit; this could be used in an algorithm, if carefully planned out, but for normal math it means the significant bits in both operands must total no more than 31.

Second, the truncated temporary result is sign-extended. This is done to a bit which was not a sign bit to begin with, and unlike other places in the specification, not documented.

This would make MACHI not an accumulator but an up/down counter with clear.

I just couldn't make out why one would want an operation shaped in such a manner. The point of a separate MAC unit is to allow concurrent pipelined operations, such as a carry save adder to handle the large width without slowing down the clock.

Apparently the RTL author also expected the straight forward implementation without any truncation, and actually added another unspecified oddity which I believe saw real world use; a shifted result fetch. This ties in to the common DSP usage of fixed point multiplication, the exact reason we commonly want the high bits of the MAC result, not the low ones. The related bug here is that the specification specifies no way to get the high bits, as the synchronisation l.macrc clears them.

In retrospect, submitting a change proposal would have been a good idea. Could someone tell me where? Meanwhile, the OR1200 RTL implements the described behaviour.

If we want the truncated behaviour, I'd like to raise the bar just a little bit: 1. Specify the sign extension behaviour, as the old architecture manual did not. 2. Submit a use case where this behaviour, as opposed to the non-truncated one, is desired.

This was an old bug, and I apologise for making assumptions about it. I believe it was a design bug propagated in or1ksim but ignored in or1200. Alternatively, it is an implementation bug in or1200 and a weak specification. Thoughts?