Bug 363 - inconsistency between isel and mfcr and crand unit test on bit-ordering of CR
Summary: inconsistency between isel and mfcr and crand unit test on bit-ordering of CR
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Mac OS
: --- enhancement
Assignee: Michael Nolan
URL:
Depends on:
Blocks:
 
Reported: 2020-06-04 20:51 BST by Luke Kenneth Casson Leighton
Modified: 2020-06-07 21:31 BST (History)
1 user (show)

See Also:
NLnet milestone: ---
total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0
parent task for budget allocation:
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Kenneth Casson Leighton 2020-06-04 20:51:11 BST
very weird.

when storing the bits in the CR (full_cr vs individual CRs) the ordering
is wrong.
Comment 1 Luke Kenneth Casson Leighton 2020-06-04 20:55:15 BST
*sigh*.  Because POWER.
Comment 2 Luke Kenneth Casson Leighton 2020-06-04 21:52:16 BST
reopening.  crand inconsistency as well.
Comment 3 Luke Kenneth Casson Leighton 2020-06-05 04:55:06 BST
michael, i kinda need your help on this one.  soc/simple/test/test_core.py
is showing reg-reordering.  this *might* be possible to "fix" in VirtualRegPort
by inverting the concatenation order of the 8 registers to create the full
32-bit, rather than doing this in power_regspec_map.py:

        if name == 'full_cr': # full CR
            return e.read_cr_whole, 0b11111111, 0b11111111
        if name == 'cr_a': # CR A
            return e.read_cr1.ok, 1<<(7-e.read_cr1.data), 1<<(7-e.write_cr.data)

however i honestly have no idea what i'm doing, and would continue to experiment
for several days, inverting bits until something worked.

or, it could just be that the CR regfile is not getting written to properl
(as in, the write-enable signals are not properly getting through).
i'll investigate that angle.
Comment 4 Luke Kenneth Casson Leighton 2020-06-05 14:39:32 BST
p34 section 2.4.1 of spec

Instructions are provided to perform logical operations on individual CR bits and to test individual CR bits.
For all fixed-point instructions in which Rc=1, and for addic., andi., and andis., the first three bits of CR Field 0 (bits 32:34 of the Condition Register) are set by signed comparison of the result to zero, and the fourth bit of CR Field 0 (bit 35 of the Condition Register) is copied from the SO field of the XER. “Result” here refers to the entire 64-bit value placed into the target register in 64-bit mode, and to bits 32:63 of the 64-bit value placed into the target register in 32-bit mode.
Comment 5 Michael Nolan 2020-06-05 14:58:28 BST
Luke - I'll investigate this at some point this weekend. I'm a little busy so I don't know exactly when
Comment 6 Luke Kenneth Casson Leighton 2020-06-05 20:41:32 BST
i am wondering if, in isacaller.py, if it would not be better to set up the CR regs SelectableInt directly
Comment 7 Luke Kenneth Casson Leighton 2020-06-05 20:47:12 BST
(In reply to Michael Nolan from comment #5)
> Luke - I'll investigate this at some point this weekend. I'm a little busy
> so I don't know exactly when

no problem, thank you.  i will move on to testing XER bits instead.

ironically the int tests actually work.

bear in mind that DecodeCRIn (and out) is responsible for defining CR0 hardcoded.  weirdly, doing 7-e.read_cr1.data in power_regspec_map.py does not also require CR0 to be changed to 7
Comment 8 Luke Kenneth Casson Leighton 2020-06-07 21:19:47 BST
michael i've just been investigating by adding extra unit tests that
i believe to be reasonable:

    def test_1_mcrf(self):
        for i in range(20):
            src = random.randint(0, 7)
            dst = random.randint(0, 7)
            lst = [f"mcrf {src}, {dst}"]
            cr = random.randint(0, (1<<32)-1)
        self.run_tst_program(Program(lst), initial_cr=cr)

    def test_0_mcrf(self):
        for i in range(8):
            lst = [f"mcrf 5, {i}"]
            cr = 0xfeff0001
            self.run_tst_program(Program(lst), initial_cr=cr)

(i used numbering to get them to run first)

test_0_mcrf one fails straight away (even on the first loop):
    self.assertEqual(expected_cr, real_cr, code)
AssertionError: 14 != 15 : mcrf 5, 0

a clue is here:


    def op_mcrf(self, CR):
        print ("op_mcrf", hex(CR.si.value), "BF", BF, "BFA", BFA)
        CR.si[(4 * BF) + 32:(4 * BF) + 35 + 1] = CR.si[(4 * BFA) + 32:(4 * BFA) + 35 + 1]
        print ("op_mcrf", hex(CR.si.value))


this prints out:

op_mcrf 0xfeff0001 BF 5 BFA 0
op_mcrf 0xfeff0f01

and... oink, the write_cr is invalid.  it's set to 1.

            cr_sel = yield dec2.e.write_cr.data
            expected_cr = simulator.cr.get_range().value
            print(f"CR whole: {expected_cr:x}, sel {cr_sel}")

CR whole: feff0f01, sel 1

ah HA!

--- a/src/soc/decoder/power_decoder2.py
+++ b/src/soc/decoder/power_decoder2.py
@@ -445,7 +445,7 @@ class DecodeCROut(Elaboratable):
                 comb += self.cr_bitfield.data.eq(0)
                 comb += self.cr_bitfield.ok.eq(self.rc_in) # only when RC=1
             with m.Case(CROutSel.BF):
-                comb += self.cr_bitfield.data.eq(self.dec.FormX.BF[0:-1])
+                comb += self.cr_bitfield.data.eq(self.dec.FormX.BF)
                 comb += self.cr_bitfield.ok.eq(1)
             with m.Case(CROutSel.BT):

that's it!  that's all it was.  what a pain.
Comment 9 Michael Nolan 2020-06-07 21:25:20 BST
GRR. I believe at some point I used the fields directly generated from power_fields.py (fields_x['BF'] or something like that), which needed the [0:-1]. But since it got changed to use the signals from the decoder, the [0:-1] chopped off the top bit. I think I fixed another error in the decoder just like that recently, I probably should have gone through and made sure there weren't any like it.
Comment 10 Luke Kenneth Casson Leighton 2020-06-07 21:31:57 BST
(In reply to Michael Nolan from comment #9)
> GRR. I believe at some point I used the fields directly generated from
> power_fields.py (fields_x['BF'] or something like that), which needed the
> [0:-1]. But since it got changed to use the signals from the decoder, the
> [0:-1] chopped off the top bit.

ohh... :)

> I think I fixed another error in the decoder
> just like that recently,

i spotted that BFA didn't have [0:-1] on it, which was partly the clue
which led me to try the same on BF.

got there in the end.  soc/simple/test/test_core.py now passes and
it runs *all* unit tests.  still TODO, the XER checking though