[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 #26 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Dmitry Selyutin from comment #25)
> Based on the discussion, refactored this way:
> https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/
> decoder/power_insn.py;h=e13d1c267432fa02f9d6893ba85a5d042c2c7715;
> hb=3256cc5c9fa4a003655bd3aba06c32a6a5f4b237#l315.
> 
> Rationale: we should not expose the internals of Instruction, the user
> should have no assumptions on the instruction components ("insndb.csv"
> entry, scalar CSV entry or SVP64 entry). These are hidden details. All these
> parts are our intimate knowledge.

 7 import pathlib as _pathlib

underscores in front of everything gets awkward to read, quite fast,
esp. when there are a lot of them.

no need to prepend with underscores, use __all__
https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

although it is supposed to be for wildcard only at least it gives
a way to declare what is public API.

i realised yesterday evening that what you are designing is the
SQL equivalent of "SELECT * FROM insn LEFT JOIN svp64" where i
was expecting a much more selective explicit system.

i then realised that actually "SELECT * LEFT JOIN" which would
create None on svp64 entries is perfectly fine.

when handing an Instruction instance to e.g. PowerOp it is *PowerOp*
that must select the subset of Signals and Consts to be created.


btw there should be no defaults in PPCRecord.  the csv files are
required 100% to contain all entries therefore strictly speaking
the factory and the __new__ function can go, they are redundant
if all fields of PPCRecord are mandatory.

(likewise for other uses of factory if all arguments are given,
 i.e. factory as best i can tell creating missing default args
 and there should be none)



  93             for bit in pattern:
  94                 value |= (bit == "1")
  95                 value <<= 1

detecting zero is missing and this will cause problems.
really the pattern should be:

mask = 0
value = 0
for bit in pattern:
    value |= (bit == "1")
    mask |= (bit != '-')
    value <<= 1
    mask <<= 1
return (mask, value)

and then of course in finding an opcode match do:

    if (opcode & mask) == value: # match

in other words you *must* also match against zeros, which are missing
from lines 93-95.

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