[Libre-soc-bugs] [Bug 838] sync or at least statically check fields.text, power_decoder, trans/svp64, CSVs between each other

[bugzilla-daemon at libre-soc.org]

https://bugs.libre-soc.org/show_bug.cgi?id=838

--- Comment #44 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Dmitry Selyutin from comment #42)
> Yet another inconsistency spotted, this time for Rc stuff.
> 
> Here are the instructions which are marked as RC.ONE (with the names exactly
> as they appear in our CSVs, I don't update the names for RC.ONE): ['lwbrx',
> 'addic.', 'andi.', 'andis.', 'stbcx', 'stdcx', 'sthcx', 'stwcx'].
> 
> 
> First, binutils names don't match in case of stbcx, stdcx, sthcx, stwcx
> instructions: in binutils, they all are marked with dot (stdbcx., stdcx.,
> etc.). It looks like we got it wrong, spec also talks of Rc marks, like
> binutils. I suggest marking these with dot explicitly, exactly like andi. or
> addic. are marked.

No. Really. The dependencies are quite mad, changes like that
require absolutely massive testing. I outlined in comment #35

Please understand and accept that those CSV files are now
effectively part of the definition of the Power ISA, or a
representation of it.

> 
> Second, some instructions are marked as RC in CSVs, but their forms lack Rc
> field. Here's the form mapping for the instructions which have this problem:
> andis.: B-FORM
> addic.: D-FORM
> andi.: B-FORM

Look at the lines. Let's pick addic


  .,......................vRCv.............
  14 12,ALU,OP_ADD,RA,0,0,NONE,0,0,addic,D,
  15 13,ALU,OP_ADD,RA,0,0,ONE,0,0,addic.,D,

that tells PowerDecoder2 that in the case of a match on
EXT012 you MUST NOT set Rc=1. Likewise on EXT013 you MUST
set Rc=1.

only when RC=RC must you look up an Rc field in the Forms.

See DecodeRC.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_decoder2.py;h=a0ebf7f86632fc94399f71139967f69d8074a4b4;hb=HEAD#l524

 540         # select Record bit out field
 541         with m.Switch(self.sel_in):
 542             with m.Case(RC.RC):
 543                 comb += self.rc_out.data.eq(self.dec.Rc)
 544                 comb += self.rc_out.ok.eq(1)
 545             with m.Case(RC.ONE):
 546                 comb += self.rc_out.data.eq(1)
 547                 comb += self.rc_out.ok.eq(1)
 548             with m.Case(RC.NONE):
 549                 comb += self.rc_out.data.eq(0)
 550                 comb += self.rc_out.ok.eq(1)


> Neither D-FORM nor B-FORM have Rc bit present.

yyep. Because RC=ONE (or NONE) that is what Rc is set to.
see above.  Lines 545 and 548.

NOT repeat NOT line 542.

You are still thinking everything must have an Rc field
if RC is set.


> These entries in binutils
> seem special and simply define OP_MASK as mask (that is, the mask for the
> major code: (0x3f << 26). 

yes. that is what happens in power_decoder() as well.  PowerDecoder hits the
first level of SwitchCase, matches,
and needs go no further. Because it is a MAJOR entry.

>I'm not sure how to do it correctly since we
> calculate the opcode based on the form;

No that is absolutely not how PowerDecoder works.  Those
values in column 1 plus the PARENT start,end ARE enough
to perform the required match (2 levels for minor)
Absolutely no need whatsoever to get the Form at that
first stage.

AFTER a match you get the CSV row which tells you what the
Form is.  One of the near to last columns.
  14 12,ALU,OP_ADD,RA,0,0,NONE,0,0,addic,D,

Yeah D. Second to last.

> I could hack the code based on
> instruction names, but perhaps I miss something and I can do it in a generic
> way.

Match all the list patterns , get the OP_XXXX and look up
the string from there.  Or just use the comment field which
is the instruction name.

there is logic in ISACaller which reconstructs addeo and
adde. from just adde. Can't remember where right now



> 
> Third, I'm not sure whether it's correct that we have "attn" instruction
> marked with RC (extra.csv).

It is not documented. Again it comes straight from Microwatt.
I cannot remember but again it cannot be removed, ISACaller
needs it.


> Binutils don't have "attn.", and also this is
> yet another instruction which can have Rc mark but has no specific form,
> therefore cannot have Rc field (where should it belong?).

the purpose here is as far as binutils is concerned
Is to identify SVP64 instructions. With attn not being
Vectorised it can be ignored.

-- 
You are receiving this mail because:
You are on the CC list for the bug.