OpenCores
Issue List
decode: CARRY_NOT_KEEP #6
Closed cowkleaver opened this issue about 14 years ago
cowkleaver commented about 14 years ago

Unless I am mistaken, there appears to be a bug in decode relating to the carry bit control. It seems the default behavior should be assigned as KEEP instead of NOT_KEEP.

The default assignments in the comb and reg processes should be modified:

  •    v.vctrl_ex.carry_keep := CARRY_NOT_KEEP;
  •    v.ctrl_ex.carry_keep := CARRY_KEEP;

And the NOT_KEEP state should be added to the shift decode: v.ctrl_ex.alu_op:= ALU_SHIFT;

  •            v.ctrl_ex.carry_keep := CARRY_NOT_KEEP;
               CASE instruction(6 DOWNTO 5) IS

I can provide an example that appears to trigger this if necessary.

takar commented about 14 years ago

I think your interpretation of the carry_keep signal is not correct - at least this is not a bug. Only the arithmetic instructions add and sub (and all its variants) have a special behavior to keep the state of the carry bit regardless of its outcome. I have confirmed this by studying <a href="http://www.xilinx.com/support/documentation/sw_manuals/mb_ref_guide.pdf">the reference manual</a>.

On the other hand, if you take a look at the shift instruction there is nothing mentioned about a carry keep functionality.

Nevertheless, the default behavior is an off-state (CARRY_NOT_KEEP) and when necessary (i.e. add(i)k(c) and rsub(i)k(c) instructions) this bit will be asserted.

takar closed this about 14 years ago
cowkleaver commented about 14 years ago

I compared the operation of MB-Lite to a reference Microblaze design and found that this does seem to be an issue. Consider the case where you perform addkc and then addc (the intent being a 64 bit addition). This CARRY_NOT_KEEP case will generate an improper result if an interrupt is taken between these instructions (carry bit is lost). The compiler may always put the instructions back to back, but someone doing assembly may not and an interrupt would definitely cause problems.

Using the reference guide as justification, the only instructions that shall (not may) modify the value of the carry bit (MSRC ? CarryOut) are ADD,SUB,SR,GET,PUT* and MTS/MSRSET. This can be verified by referencing the 'Registers Altered' sections of instructions. The current implementation would require the MSRC bit be listed for every instruction.

takar commented about 14 years ago

I will again take a look at this issue. Anyway, I can not re-open this bug but don't be afraid :)

I'm considering to include the MSR-register and instructions to make it even more architecturally correct.

cowkleaver commented about 14 years ago

Thanks. Sounds good.

ALso, I have an implementation of the MSR register and the infrastructure required for implementation of other SPRs. If you're interested I can pass those on to you as a starting point. Right now only the MSR is implemented.

takar commented about 14 years ago

I think I owe you an apology, since you are absolutely right about this issue! Thanks for your perseverance on this one...

The problem does indeed occur when you would try to use the carry bit <i>not</i> immediately, but several instructions after it was generated...

By the way, I didn't get this from your initial description (trying to safe my own soul here ;) )

If you would like to share your MSR implementation that would be great, than I don't have to "reinvent the wheel" and official releases can be expected faster.


Assignee
No one
Labels
Bug