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.
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?
Created attachment 125 [details] suggested fixes for MSB0 issues in SVP64
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
(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.
(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.
(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.
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