Bug 600 - Fix MSB0 issues in the SVP64 Assembler, Simulator and Decoder
Summary: Fix MSB0 issues in the SVP64 Assembler, Simulator and Decoder
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- enhancement
Assignee: Cesar Strauss
URL:
Depends on:
Blocks:
 
Reported: 2021-02-16 14:22 GMT by Cesar Strauss
Modified: 2021-02-21 17:46 GMT (History)
2 users (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
suggested fixes for MSB0 issues in SVP64 (7.60 KB, patch)
2021-02-16 15:08 GMT, Cesar Strauss
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cesar Strauss 2021-02-16 14:22:55 GMT
While investigating failures in test_issuer_svp64.py, I began noticing issues in the decoder when extracting the SVP64 prefix fields, due to mistakes in MSB0 handling. Eventually, I found related issues in the assembler and simulator as well.

I think I managed to find and fix all such issues, locally. I did this by checking, in GTKWave, the instruction memory against the expected hand assembled instruction, and the decoded fields against the original instruction. I kept fixing issues in the decoder, assembler and simulator, until test_issuer_svp64.py passed.

Before I commit, I think maybe it would be better to first create a test case, to confirm that the current version really has issues, and the fixes are not introducing new bugs instead of fixing them.

The plan for this test case is to:

1) Have some list of SVP64 instructions (and a few non-SVP64 instructions as well)
2) Transform to v3.0B assembly, and compile
3) Check the binary result against the expected result, which was hand assembled according to the SVP64 specification.
4) Pass the instruction to the decoder
5) Check decoded fields against the original instruction.
Comment 1 Luke Kenneth Casson Leighton 2021-02-16 14:50:03 GMT
i really am getting fed up of MSB0.  it is going to be a major hindrance
in the v3.0B pseudocode with the proliferation of 63-i here, 31-i there.

i had forgotten to add this:

--- a/src/soc/decoder/isa/caller.py
+++ b/src/soc/decoder/isa/caller.py
@@ -686,6 +686,7 @@ class ISACaller:
         yield self.dec2.dec.bigendian.eq(self.bigendian)
         yield self.dec2.state.msr.eq(self.msr.value)
         yield self.dec2.state.pc.eq(pc)
+        yield self.dec2.state.svstate.eq(self.svstate.spr.value)
 
         # SVP64.  first, check if the opcode is EXT001, and SVP64 id bits set
         yield Settle()

which when i do so, because the bit-order is inverted the result is that
the register numbers get cranked up to 41 (the .v is mis-interpreted)

please bear in mind that ISACaller - and the SelectableInt class - actually
does MSB0 inversion automatically.  this is its job.

therefore technically speaking it is ISACaller that is "correct", and
PowerDecoder2 (or more specifically SVP64PrefixDecoder's interpretation
of the sv_rm field) that is likely to be wrong.

as in: although the bit-numbering has been inverted this may have also
inverted the order of the RM bits.

        rmfields = [6, 8] + list(range(10,32)) # SVP64 24-bit RM
        l = []
        for idx in rmfields:
            l.append(self.opcode_in[32-idx]) 
        with m.If(self.is_svp64_mode):
            comb += self.svp64_rm.eq(Cat(*l))

should be

        rmfields = [6, 8] + list(range(10,32)) # SVP64 24-bit RM
        l = []
        for idx in rmfields:
            l.append(self.opcode_in[32-idx]) 
  --->  l.reverse()
        with m.If(self.is_svp64_mode):
            comb += self.svp64_rm.eq(Cat(*l))

what's your thoughts there?

can you attach a patch (diff) here so i can take a look?
Comment 2 Cesar Strauss 2021-02-16 15:08:39 GMT
Created attachment 125 [details]
suggested fixes for MSB0 issues in SVP64
Comment 3 Luke Kenneth Casson Leighton 2021-02-16 15:34:41 GMT
Comment on attachment 125 [details]
suggested fixes for MSB0 issues in SVP64

23-i there.  otherwise looks good.

p.s. did i say i can't stand MSB0?

@@ -544,7 +544,7 @@ class SVP64Asm:
             svp64_prefix |= 0x1 << (31-9) # SVP64 marker 2
             rmfields = [6, 8] + list(range(10,32)) # SVP64 24-bit RM
             for i, x in enumerate(rmfields):
-                svp64_prefix |= ((svp64_rm>>i)&0b1) << (31-x)
+                svp64_prefix |= ((svp64_rm>>(31-i))&0b1) << (31-x)

             # fiinally yield the svp64 prefix and the thingy.  v3.0b opcode
             yield ".long 0x%x" % svp64_prefix
Comment 4 Cesar Strauss 2021-02-17 12:33:44 GMT
(In reply to Luke Kenneth Casson Leighton from comment #3)
> 23-i there.  otherwise looks good.

Thanks for the review, I have applied the patch. I guess we can leave the test case for later.

I'll now proceed to use the methods of soc/consts.py to replace all the MSB-i instances with big-endian based constants.
Comment 5 Cesar Strauss 2021-02-21 11:22:31 GMT
(In reply to Cesar Strauss from comment #4)
> I'll now proceed to use the methods of soc/consts.py to replace all the
> MSB-i instances with big-endian based constants.

I think I converted all hardcoded instances of MSB-i in the original patch to symbolic.

Now, I will move the field selection function to nmutil, making it accept a Module for adding combinatorial logic, and returning a Signal. That way, it becomes explicit that the function really adds wires to the design.
Comment 6 Cesar Strauss 2021-02-21 17:40:39 GMT
(In reply to Cesar Strauss from comment #5)
> Now, I will move the field selection function to nmutil, making it accept a
> Module for adding combinatorial logic, and returning a Signal. That way, it
> becomes explicit that the function really adds wires to the design.

Done.

Nothing else to do here, I think.

It still would be nice to have an unit test for the SVP64 assembler and decoder, eventually.
Comment 7 Luke Kenneth Casson Leighton 2021-02-21 17:46:55 GMT
agreed.  at the moment it is inspection by hand (execute svp64.py) and the testissuer runs then indirectly confirm working.

what we really need though is an objdump, because then the assemble-then-objdump should be exactly the same