[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 #28 from Dmitry Selyutin <ghostmansd at gmail.com> ---
(In reply to Luke Kenneth Casson Leighton from comment #26)
> (In reply to Dmitry Selyutin from comment #25)
> 
>  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__

I'm aware of __all__. This serves a completely different purpose (though also
somewhat helps to achieve the same goal as __all__): I want to be explicit
about what is imported and what is local, and don't want to shadow anything.
`csv = csv.CSVReader(...)` is just the very first example which comes to my
mind.

> 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.

I've never worked with SQL so I can neither confirm or deny it.

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

Yes.

> btw there should be no defaults in PPCRecord.  the csv files are
> required 100% to contain all entries

This is incorrect, both from CSVs point of view and sources.

CSVs (only a pair of entries, there're others):
-----11111,FPU,OP_FP_MADD,FRA,FRB,FRC,FRT,NONE,CR1,0,0,ZERO,0,NONE,0,0,0,0,1,0,RC,0,0,fnmadds,A,,,
0b1001000000,,,,,,,,,,,,,,,,,,,,,,,mcrxrx,X,

Code: src/openpower/decoder/power_enums.py
default_values = {'unit': "NONE", 'internal op': "OP_ILLEGAL",
                  'in1': "RA", 'in2': 'NONE', 'in3': 'NONE', 'out': 'NONE',
                  'CR in': 'NONE',
                  'ldst len': 'NONE',
                  'upd': '0',
                  'rc': 'NONE', 'cry in': 'ZERO', 'form': 'NONE'}

I'm totally fine if you update the CSV respectively. In that case, these
defaults won't be needed. Until then, there's a need for an explicit check for
null string upon constructing *Record instance. So, if you don't want the
default values, values must be present.

> and then of course in finding an opcode match do:
> 
>     if (opcode & mask) == value: # match

Clarified in IRC; however, since we're going to need it anyway, I'll either
have some class with opcode and mask properties, or simply will extend the
Opcode with mask property.

(In reply to Luke Kenneth Casson Leighton from comment #27)
>  364             fields += [(self.__section.opcode, 0, 5)]
> 
> that can be converted to (mask, value) form

I took it from binutils (instruction() function). I'd prefer this form as it
fits fields.text better, and is really obvious (unlike masks).

>  368         if self.name.endswith("."):
>  369             fields += [(1, 31, 31)]
> 
> ah no.  definitely not.
> 
> 1) some Rc=1 instructions *do not* use bit 31
> 2) some instructions (stcix. or atomics somewhere) it is implicit
>    (i.e. there is no non-Rc version)
> 3) Rc=1 field extraction is often done elsewhere i.e. by manual
>    recognition of the bit.

Thanks for this clarification! It seems we cannot avoid dealing with
fields.text. That said... this is also part of information about the
instruction, so it's reasonable.

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