Currently, whenever fields.text is edited, multiple files around must also be reflected in multiple other files. It's easy to miss the relevant changes. Ideally the changes, once completed, should be kept to sync. Perhaps there must be a different form to store the relevant information, which generates some of the files we use now; svp64.py should likely generate the instructions automatically; and so on. At the very minimum, we must at least trigger that something became inconsistent (but ideally we should simply follow SSOT).
svp64.py basically should be updated to use PowerFields() to autogenerate the binary opcode. but, it's not quite that straightforward: there is still information in power_decoder.py which is manual. here: https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_decoder.py;h=39ba5f22a46c35ed3824faacb9bad8c96d43f396;hb=HEAD#l737 737 Subdecoder(pattern=22, opcodes=get_csv("minor_22.csv"), 738 opint=True, bitsel=(1, 5), suffix=None, subdecoders=[]), this is *manually* specified unfortunately, with bitsel=(1,5) matching the size of the CSV entries: https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=openpower/isatables/minor_22.csv;h=9fc38e024d34b62e4a715dc4299ea9436fdbc4fa;hb=318e981331df094fe1b46e796656993ccef38a1c 5 0b00011,VL,OP_SVSTEP,.... this is going to be a pain so probably a static checker is best
i'm putting this under binutils as a sub-bug because by writing unit tests for binutils the issue here can be fixed along the way, it's related enough to justify.
temporary (intermediary) fixes in svp64.py going in https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=b022857118371b5f60dadfbffd00f3ab41ee1b61
(In reply to Luke Kenneth Casson Leighton from comment #3) > temporary (intermediary) fixes in svp64.py going in > https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff; > h=b022857118371b5f60dadfbffd00f3ab41ee1b61 enough unit tests run now to show these are pretty good. fixed a couple of errors along the way, but we should have had proper svp64.py unit tests in the first place, sigh.
see https://bugs.libre-soc.org/show_bug.cgi?id=845#c6 how to identify XO pattern. TopPowerDecoder is an instance of PowerDecoder which contains a list member dec containing namedtuples Subdecoder(). it's a tree-hierarchy in other words and in *theory* it can be flattened by converting the top-level bit-pattern to binary and ORing each one with the child's bit-patterns. note that it is the *parent* that declares the bitsel and opint of its *children* (opcodes).
https://bugs.libre-soc.org/show_bug.cgi?id=845#c8 Everything said here also applies to this task. When designing the single source of truth, a great care must be taken so that high-level concepts do not find their way to low-level ones. That is, at the point where we deal with instructions, there must be no verilog, nmigen or other stuff. Only pure Python with builtin modules. Then each and every high-level concept, be it binutils, simulator or anything else, should use these "bricks" and wrap them. binutils code generation or nmigen should not affect each other.
(In reply to Dmitry Selyutin from comment #6) > https://bugs.libre-soc.org/show_bug.cgi?id=845#c8 > > Everything said here also applies to this task. should not be discussing there. should be discussing here, only.
i can live with a bitsel.csv file which contains the offsets needed to be able to interpret the CSV files column 1. file,start,end,int minor_22,21,31,1 something like that.
Here are patterns which break my parser, and I don't understand how this works in decode_extra et al. csv insn pattern openpower/isatables/LDSTRM-2P-2S1D.csv stdux s:RSs:RA openpower/isatables/LDSTRM-2P-2S1D.csv stwux s:RSs:RA openpower/isatables/LDSTRM-2P-2S1D.csv stbux s:RSs:RA openpower/isatables/LDSTRM-2P-2S1D.csv sthux s:RSs:RA openpower/isatables/LDSTRM-2P-2S1D.csv stfsux s:FRSs:RA openpower/isatables/LDSTRM-2P-2S1D.csv stfdux s:FRSs:RA openpower/isatables/LDSTRM-2P-3S.csv stwcx s:RSd:CR0 openpower/isatables/LDSTRM-2P-3S.csv stdcx s:RSd:CR0 openpower/isatables/LDSTRM-2P-3S.csv stbcx s:RSd:CR0 openpower/isatables/LDSTRM-2P-3S.csv sthcx s:RSd:CR0 I'd have expected at least yet another semicolon here, e.g. s:RS;d:CR0.
(In reply to Dmitry Selyutin from comment #9) > I'd have expected at least yet another semicolon here, e.g. s:RS;d:CR0. totally bodged it. commit 43b06b819839a3b84a76fb69a5b3690918e6b381 (HEAD -> master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Wed Aug 3 01:42:10 2022 +0100 completely bungled multi-EXTRA specs https://bugs.libre-soc.org/show_bug.cgi?id=838#c9 should be d:RS;d:CR0, missing a semicolon. sigh
This is why I think shiny classes are better. The code made an impression it worked, but actually it ignored illegal arguments (broken EXTRA patterns, TODO in fishmv, etc.).
(In reply to Dmitry Selyutin from comment #11) > This is why I think shiny classes are better. gimme sparkly. > The code made an impression it > worked, but actually it ignored illegal arguments (broken EXTRA patterns, naah. that's just me taking sbortcuts to get s*** done fast.
0b0011000000,,,RA,RB,NONE,NONE,NONE,BF,,,,,,,,,,,,,,,cmprb,X, 0b1011110011,,,,,RT,,,,,,,,,,,,,,,,,,darn,X, 0b1000000000,,,,,,,,,,,,,,,,,,,,,,,mcrxr,X, 0b1001000000,,,,,,,,,,,,,,,,,,,,,,,mcrxrx,X, These are very, very wrong. Having these in CSV is bad, and, again, the fact that these are silently processed and appear to work makes it even worse. Any objections if I drop these?
(In reply to Dmitry Selyutin from comment #13) > Any objections if I drop these? Or, alternatively, please update these with something relevant. I don't think leaving the columns empty and handling these cases in code is a good idea.
(In reply to Dmitry Selyutin from comment #13) > 0b0011000000,,,RA,RB,NONE,NONE,NONE,BF,,,,,,,,,,,,,,,cmprb,X, > 0b1011110011,,,,,RT,,,,,,,,,,,,,,,,,,darn,X, > 0b1000000000,,,,,,,,,,,,,,,,,,,,,,,mcrxr,X, > 0b1001000000,,,,,,,,,,,,,,,,,,,,,,,mcrxrx,X, > > These are very, very wrong. Having these in CSV is bad, and, again, the fact > that these are silently processed and appear to work makes it even worse. > Any objections if I drop these? they haven't been properly analysed yet. so much to do. sv_analysis.py skips darn. no absolutely they cannot be removed because the simulator MUST implement them in scalar. remember these CSV files are EXECUTABLE not just for SVP64 but Scalar Power ISA v3.0. so no under NO CIRCUMSTANCES can they be "removed", it will be as if the instructions had been removed from Scalar Power ISA 3.0 (In reply to Dmitry Selyutin from comment #14) > (In reply to Dmitry Selyutin from comment #13) > > Any objections if I drop these? > > Or, alternatively, please update these with something relevant. I don't > think leaving the columns empty and handling these cases in code is a good > idea. it is what it is. they cannot be removed, sv_analysis.py has to know (somehow) and cope with the discernment "what is vectorised and what is not". likewise mfmsrd, mtmsr, rfid, sc and others cannot and must not be removed because if they are removed then that catastrophically causes ISACaller to be unable to execute those *SCALAR* instructions.
Here's what we have upon calling this code: db = Database(get_wiki_dir()) print(db["addi"]) Record(ppc=PPCRecord(opcode=0x0000000e, comment='addi', flags=Flags({'sgn', 'BR', 'inv A', 'sgn ext', 'cry out', 'inv out', 'rsrv', '32b', 'lk', 'sgl pipe'}), comment2='', unit=<Function.ALU: (1 << 1)>, intop=<MicrOp.ADD: 2>, in1=<In1Sel.RA_OR_ZERO: 2>, in2=<In2Sel.CONST_SI: 3>, in3=<In3Sel.NONE: 0>, out=<OutSel.RT: 1>, cr_in=<CRInSel.NONE: 0>, cr_out=<CROutSel.NONE: 0>, cry_in=<CryIn.ZERO: 0>, ldst_len=<LDSTLen.NONE: 0>, upd=<LDSTMode.NONE: 0>, rc=<RC.NONE: 0>, form=<Form.D: 4>, conditions='', unofficial=False), svp64=SVP64Record(identifier='addi', ptype=<SVPtype.P2: 2>, etype=<SVEtype.EXTRA3: 2>, in1=<In1Sel.RA_OR_ZERO: 2>, in2=<In2Sel.NONE: 0>, in3=<In3Sel.NONE: 0>, out=<OutSel.RT: 1>, out2=<OutSel.NONE: 0>, cr_in=<CRInSel.NONE: 0>, cr_out=<CROutSel.NONE: 0>, extra={0: [d:RT, d:CR0], 1: [s:RA], 2: [], 3: []}, pu=False, conditions='', mode=<SVMode.NORMAL: 1>)) The PPCRecord and SVP64Record correspond to CSV entries, with all paranoid checks. All the fields there, except for comment and comment2 fields, even those shown as strings and integers, are real custom types with paranoid checks for the input. Both PPCRecord and SVP64Record are put into the Record class (and SVP64Record can be optional). The Record class will handle the task of calculating the whole opcode, decoding extras, etc.
Implemented remapping from register types to extra indices (i.e. from In#Sel/OutSel classes to SVEXTRA class). Now available in Record class.
Whoa. 3 files changed, 686 insertions(+), 98 deletions(-), and I haven't yet even replaced the original stuff. The progress can be checked at binutils branch. This progress includes: 1) the new special CSV which maps bitsel to other CSVs; 2) many changes and updates to power_enums (mostly new classes and constructors); 3) power_insn.py module which implements PPCRecord, SVP64Record and the class which combines these, plus the aforementioned Database class.
This seems to do the trick. A bit hacky, and I'm not sure of this entirely, but it did achieved the results for some codes I checked in binutils. class Record: ppc: PPCRecord svp64: SVP64Record section: Section @property @_functools.lru_cache(maxsize=1) def opcodes(self): fields = [] (start, end) = self.section.bitsel if self.section.opcode: fields += [(self.section.opcode, 0, 5)] fields += [(self.ppc.opcode, start, end)] else: fields += [(self.ppc.opcode, start, end)] yield Opcode(fields) if self.ppc.rc is RC.RC: yield Opcode(fields + [(1, 31, 31)]) I'm not sure of whether we should output it this way; the thing is that we have one record shared for two instruction, with Rc set and without. Perhaps this should be refactored a bit, these are distinct instructions; it's only our CSVs which happen to treat these as a single entity (that said, they're also correct, because logically it's one entry). Anyway, I'll proceed to replacing some bits soon. I'll start with binutils, because I don't want to break anything. Some code will be removed, especially the part where we call get_svp64_csv manually on well-known CSVs. This will be replaced with the new database and all classes around.
I thought about it for a while. Yes, it makes sense to distinguish these instructions (like orc and orc.) on Record level. I finally renamed Record to Instruction, since it's no longer a bunch of CSV records, but a full-blown class which encapsulates the records and adds some complex logic atop (extra remapping, opcode calculation, Rc distinction and so on). https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=refs/heads/binutils
https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=785d78964c68632dbe3a09b4b5ada4c3188df078 just going through these, no sorry this is microwatt compatibility see (entirety of) microwatt source code esp decode.vhdl, and soc, and Boris Shingarov's work, and the CSV files themselves, the names are set in stone and cannot change now. i.e. i want people looking at microwatt to be able to go, "oh, these start OP_ and are the same name therefore must be the same" rest is great.
https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_insn.py;hb=9dbf16f3aa783d95ca92ad74ebef9474c7750dd2#l122 ah, sorry, just saw this - can you please try to find another way that does not use dataclasses. the reason is not just that they restrict types but that i am endeavouring to ensure that jacob begins to respect python as a dynamic runtime language based around the Liskov Substitution Principle. dataclasses, by being like structs of other languages, make a mockery of that strength of LSP. a good alternative would be to turn that list of name-types into a dictionary and then use setattr() again, here: 313 class Instruction: 314 section: Section 315 ppc: PPCRecord 316 svp64: SVP64Record 317 rc: bool = False please do find another way to do that which does not use dataclass. i do not want us to have to spend the money explaining to a team of *HDL* engineers not used to advanced python programming what the hell dataclasses are.
(In reply to Luke Kenneth Casson Leighton from comment #21) > https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff; > h=785d78964c68632dbe3a09b4b5ada4c3188df078 > > just going through these, no sorry this is microwatt compatibility > see (entirety of) microwatt source code esp decode.vhdl, and soc, and > Boris Shingarov's work, and the CSV files themselves, the names > are set in stone and cannot change now. First, this isn't used in Microwatt or decode.vhdl or by Boris Shingarov. This is an internal representation. It could have been anything as long as it's clear how the conversion happens. It's like stating that "these are strings in decode.vhdl, so you cannot treat it as integer in Python". Sorry, I'm not buying this argument. > i.e. i want people looking at microwatt to be able to go, "oh, these > start OP_ and are the same name therefore must be the same" This is more valid argument, because it's not "greppable". As I mentioned, OP_ prefix is redundant to keep in the internal representation. So it's dropped, but still enum works it out via _missing_ method. I can make it more explicit and introduce a mapping like {"OP_A": MicrOp.A, "OP_B": MicrOp.B}. Or even in the enum itself, OP_A = A. I'd be surprized if it's difficult to find what happens even now, but I can do it for explicitness. (In reply to Luke Kenneth Casson Leighton from comment #22) > https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/ > decoder/power_insn.py;hb=9dbf16f3aa783d95ca92ad74ebef9474c7750dd2#l122 > > ah, sorry, just saw this - can you please try to find another way that > does not use dataclasses. the reason is not just that they restrict > types but that i am endeavouring to ensure that jacob begins to respect > python as a dynamic runtime language based around the Liskov Substitution > Principle. dataclasses, by being like structs of other languages, > make a mockery of that strength of LSP. Apparently you had the discussion about it with Jacpb, and I'm not aware of it. When I see arguments like "it violates the LSP", I need the "why" and "how". IRC logs or links to tasks or mailing lists are appreciated. > a good alternative would be to turn that list of name-types into a > dictionary and then use setattr() > please do find another way to do that which does not use dataclass. Rationale. > i do not want us to have to spend the money explaining to a team > of *HDL* engineers not used to advanced python programming what the > hell dataclasses are. If HDL engineers is not that advanced to type "python dataclass" in Google, I'd say they're not engineers at all. But, anyway: there's nothing "advanced", it's in fact a simple dict, with some sugar atop.
(In reply to Dmitry Selyutin from comment #23) > This is more valid argument, because it's not "greppable". As I mentioned, > OP_ prefix is redundant to keep in the internal representation. So it's > dropped, but still enum works it out via _missing_ method. I can make it > more explicit and introduce a mapping like {"OP_A": MicrOp.A, "OP_B": > MicrOp.B}. Or even in the enum itself, OP_A = A. I'd be surprized if it's > difficult to find what happens even now, but I can do it for explicitness. https://git.libre-soc.org/?p=openpower-isa.git;a=blobdiff;f=src/openpower/decoder/power_enums.py;h=03987fb774f0fd4171bbe4883c86c8eb55dc828f;hp=2191f83a0cde7329e722b81b1c9695d40c14f1d1;hb=3f36bfca31abd8d5f09131d7c09554a8bdd82cf5;hpb=dc7bc2e53cff0b5bc442a15955c61bc825d3b69c
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.
(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.
364 fields += [(self.__section.opcode, 0, 5)] that can be converted to (mask, value) form. opcode, 0b111111 although a case can be made for keeping the start/end thing and maybe even converting mask *into* start and end. 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.
(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.
(In reply to Dmitry Selyutin from comment #28) > I've never worked with SQL so I can neither confirm or deny it. intuitively it works as it sounds, all fields are wildcard included. > > 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, hmmmmm.... yep, hadn't spotted / forgot that. been a while. > > 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. if the list is compacted by a function down into a single tuple of 32 bit (mask, value) by reconstruction the PowerDecoder may be drastically simplified to a single for-loop creating one single giant nmigen Switch/Case onto a Memory (ROM) lookup. 0,6, MAJOR would map to 0b0000...0111111 0b0000...NNNNNN and be ORed with other entries in the list, similarly mapped. > (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. power_fields.text, yes. you have to perform a lookup of the instruction mnemonic first, followed by finding the Form (X-Form, D-Form) *then* you can look up Fields reliably. (this is why decoding is done in two stages, decode1 and decode2) bear in mind that this all comes from microwatt decode1.vhdl and decode2.vhdl. those HDL files were created so as to not have Absolute Hell On Earth which if you have seen gem5's hand-created Power ISA decoder tens of thousands of lines long with nested switch statements you start to appreciate. however.... decode1.vhdl was NOT designed to be absolute 100% fully automated i.e. equivalent to binutils objdump. thus *neither are the csv files*. an example is mtmsr[d] there is *only one OP*, OP_MTMSR and inside the PIPELINE the required bit, L, is MANUALLY decoded to discern between these two instructions. the RC column in the CSV files is expected to be used in this same way and *also includes OE detection* as a note *to the PIPELINE* (and also decode2.vhdl aka PowerDecoder2) to perform further *manual* decoding of both Rc and OE. i relate this because efforts to turn the csv files into a full automated objdump have consequences and a high cost, not just in coding terms but also deviation from microwatt. i have no problem with auto-generating extra field entries that go into class instances but drastic modification of the CSV files is to be strongly discouraged. as is using JSON or other "modern" format for the same reasons.
(In reply to Luke Kenneth Casson Leighton from comment #29) > power_fields.text, yes. you have to perform a lookup of the instruction > mnemonic first, followed by finding the Form (X-Form, D-Form) *then* > you can look up Fields reliably. bearing in mind the CSV file entries also have the Form. the reason i mention all this is because an automated encode (reading csv, getting Form, reading PowerFields, getting regs and other operands) is what the whole point of this bugreport is all about, in particular sv/trans/svp64.py it is just that if done *in general* it will not go entirely to plan (i.e. attempting to replace binutils in full), but fortunately it will be sufficient to cover only those instructions *not* currently in binutils such as ffmadds, fishmv, etc. for which i would expect there to be no "annoying quirks".
I've cleaned the code a bit, and also updated insndb CSV with opcode type (called mode; can either be "integer" or "pattern"). After a long period of thinking, I've decided to restore dataclass for Instruction, but, at the same time, keep the fields translation logic. That said, I marked svp64 field as optional, so the users know some Instructions might lack svp64 context. Next on the roadmap is fields.text and re-using this information for opcode calculations.
(In reply to Dmitry Selyutin from comment #31) > same time, keep the fields translation logic. That said, I marked svp64 > field as optional, so the users know some Instructions might lack svp64 > context. yep. sc. mtmsr. mfmsr. rfid. isync. these and similar will neverrrr be Vectorised, more than that: the prefixing if ever attempted is required to raise an illegal instruction trap. that keeps to the RISC paradigm as applied to prefix-suffix.
The table parsing is complex enough, so I decided to re-use (or, well, rather wrap) DecodeFields and postpone the cleanup in this part. It's doable and should be done, too, at least for validation's sake (also for real unmarshalling, better typing system, etc.). However, I suggest to postpone it to the later stage. Even the minor validation I'm doing now raises the questions. I suspect I found an issue, or perhaps an undocumented feature. The operands RA, RT and XO mention DQE form, which is not present in our known forms. Moreover, it's not even known to ISA 3.1b; it's only mentioned as part of the operands' description. Ideas?
(In reply to Dmitry Selyutin from comment #33) > The table parsing is complex enough, so I decided to re-use (or, well, > rather wrap) DecodeFields and postpone the cleanup in this part. i'd greatly prefer it wasn't touched at all. any time spent will apart from anything be a waste of time as when the latex version of the Power ISA spec is available we need to convert autoreading of that. that's assuming it has what is needed, Paul assures me he did his best to make it machine-parseable. > It's doable > and should be done, too, at least for validation's sake (also for real > unmarshalling, better typing system, etc.). However, I suggest to postpone > it to the later stage. i really prefer it not to be touched at all, it is quite critical and requires a full re-run of every single unit test we have. not just in the Simulator but in the HDL as well. > Even the minor validation I'm doing now raises the questions. I suspect I > found an issue, or perhaps an undocumented feature. The operands RA, RT and > XO mention DQE form, which is not present in our known forms. Moreover, it's > not even known to ISA 3.1b; it's only mentioned as part of the operands' > description. Ideas? examine v3.0B. that is what fields.txt was originally taken from. v3.1 was not available.
just so you're aware: any change to the PowerFields API affects: * SVP64Asm * ISACaller (which needs fields to be extracted) * pywriter (the compiler) * PowerDecoder2 * every single HDL pipeline * sv_analysis it's basically *literally* everything hence it makes me extremely nervous to make any changes of any kind. i.e. a "simple" innocuous change to the API would result literally in hundreds of knock-on changes to the entire codebase. it's so critical that now that i think about it, it would be better to read the latex document when available and convert it into fields.txt format rather than alter power_fields.py
This should just be done carefully. However, considering time constraints and the priorities, I won't touch this code now. After everything I saw, I'm pretty confident this must be validated, typed and better organized; this includes the whole code around. However, this is not something we should do _now_. As I said, for now I'm mostly concerned about binutils, and there's almost nothing to break here. By the way, we can check the SVP64 opcodes and masks with binutils, if some define is present. I'll do it, it'd be nice to have these cross-checks whenever possible.
(In reply to Luke Kenneth Casson Leighton from comment #34) > examine v3.0B. that is what fields.txt was originally taken from. > v3.1 was not available. It's not there either. This form is mentioned in operands but not elsewhere.
Forms missing from enumeration: VA2, SVC, SVR. Forms not known at all (both 3.0B and 3.1): DQE, TX. TX looks like an X-form with TX bit present (cf. mtvsrwz). I've updated the enumeration list: https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=08088da82e27a632f7c4c606f1ed2ea3c64658ca As for unknown forms, which are present only in operands description, -- I deliberately skip these for now in the code.
(In reply to Dmitry Selyutin from comment #38) > Forms missing from enumeration: VA2, SVC, SVR. > Forms not known at all (both 3.0B and 3.1): DQE, TX. > > TX looks like an X-form with TX bit present (cf. mtvsrwz). yes TX is the extra bit that turns PackedSIMD (VSX) reg numbers from 5-bit (32 regs) to 6-bit (64 regs). they probably put the one extra bit elsewhere, leaving the 5 bits RA RB etc. etc. where they are to avoid the MUXes just for PackedSIMD [a common hardware decode tactic] > I've updated the enumeration list: > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=08088da82e27a632f7c4c606f1ed2ea3c64658ca + VA2 = 38 + SVC = 39 + SVR = 40 ehn? how did i miss those, well-spotted :) > As for unknown forms, which are present only in operands description, -- I > deliberately skip these for now in the code. you can guess what happened, can't you - earlier versions of Power ISA spec had those (v2.06, v2.07) and someone did a cleanup of the v3.1 spec but forgot to go back and clean up v3.0
(In reply to Luke Kenneth Casson Leighton from comment #39) > yes TX is the extra bit that turns PackedSIMD (VSX) reg > numbers from 5-bit (32 regs) to 6-bit (64 regs). they > probably put the one extra bit elsewhere, leaving the > 5 bits RA RB etc. etc. where they are to avoid the MUXes > just for PackedSIMD [a common hardware decode tactic] Well we might have it as an alias to Form.X. What do you think? > ehn? how did i miss those, well-spotted :) I think these should rather appear _before_ our custom forms. I guess enumeration values do not value much for Form enum, just a logical thought. > > As for unknown forms, which are present only in operands description, -- I > > deliberately skip these for now in the code. Well I cleaned these up. Perhaps I should drop a note to them. > > you can guess what happened, can't you - earlier versions of > Power ISA spec had those (v2.06, v2.07) and someone did a cleanup > of the v3.1 spec but forgot to go back and clean up v3.0 I think we should contact them to tell about this; any email to reach them?
(In reply to Dmitry Selyutin from comment #40) > (In reply to Luke Kenneth Casson Leighton from comment #39) > > yes TX is the extra bit that turns PackedSIMD (VSX) reg > > numbers from 5-bit (32 regs) to 6-bit (64 regs). they > > probably put the one extra bit elsewhere, leaving the > > 5 bits RA RB etc. etc. where they are to avoid the MUXes > > just for PackedSIMD [a common hardware decode tactic] > > Well we might have it as an alias to Form.X. What do you think? No doesn't work that way. It is a hierarchy. Form.X.TX Form.whatever.field All of the aliases you can see in PowerFields should never have been added and under no circumstances should they be used. They were part of microwatt. NO pipelines use self.forms.RA ALL pipelines use self.forms.FormX.RA. so no, under no circumstances should any "aliases" be added. The operands are accessible ONLY via the 2 level hierarchy or not at all Now we come to a painful bit. IBM former custodians of Power ISA had the attitude that the Forms are not for machine readability and neither is the ISA. this is stupid as it costs time and money which apparently they must clearly have plenty of. If you look closely there are very unfortunately MULTIPLE fields named "L" even WITHIN the same Form. this because clearly anyone wishing to do a machine readable spec must be stupid or "well we never intended it to be, we never did it that way, so tough" was the response. Not impressed with that I *renamed* L to L1 L2 L3 to avoid conflicts and this is just how it has to be. > > ehn? how did i miss those, well-spotted :) > > I think these should rather appear _before_ our custom forms. I guess > enumeration values do not value much for Form enum, just a logical thought. I had only added the instructions needing them to the Simulator ISACaller, it is the HDL that would need them and that hasn't been done yet. ISACaller does not use Power enum Forms. > Well I cleaned these up. Perhaps I should drop a note to them. Yes there will soon be a external RFC process available Although I really want to keep track of things, they have the perspective that all and any even slightest minor revisions and corrections shall remain secretive and confidential. Which makes sense for certain scenarios but not all. > I think we should contact them to tell about this; any email to reach them? No there will be an RFC web page up shortly, which will even have a question "do you want to attach a diff patch"
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. 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 Neither D-FORM nor B-FORM have Rc bit present. These entries in binutils seem special and simply define OP_MASK as mask (that is, the mask for the major code: (0x3f << 26). I'm not sure how to do it correctly since we calculate the opcode based on the form; I could hack the code based on instruction names, but perhaps I miss something and I can do it in a generic way. Third, I'm not sure whether it's correct that we have "attn" instruction marked with RC (extra.csv). 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?).
(In reply to Dmitry Selyutin from comment #42) > as they appear in our CSVs, I don't update the names for RC.ONE): ['lwbrx', > 'addic.', 'andi.', 'andis.', 'stbcx', 'stdcx', 'sthcx', 'stwcx']. Sorry, lwbrx appeared there by mistake, don't consider it. A minor note so that it' clear which opcode values and masks I mean from binutils point of view. Let's consider entry for lwbrx. https://git.libre-soc.org/?p=binutils-gdb.git;a=blob;f=opcodes/ppc-opc.c#l8223 {"lwbrx", X(31,534), X_MASK, PPCCOM, 0, {RT, RA0, RB}}, #define X(op, xop) (OP (op) | ((((uint64_t)(xop)) & 0x3ff) << 1)) #define OP(x) ((((uint64_t)(x)) & 0x3f) << 26) #define XRC(op, xop, rc) (X ((op), (xop)) | ((rc) & 1)) #define X_MASK XRC (0x3f, 0x3ff, 1) With our code, what we get is: Instruction(name='lwbrx', opcode=FieldsOpcode(value=0x7c00042d, mask=0xfc0007ff)) I obviously removed lot of other prints here (the dataclass printed the whole fields), but you get the idea.
(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.
(In reply to Dmitry Selyutin from comment #43) > (In reply to Dmitry Selyutin from comment #42) > > as they appear in our CSVs, I don't update the names for RC.ONE): ['lwbrx', > > 'addic.', 'andi.', 'andis.', 'stbcx', 'stdcx', 'sthcx', 'stwcx']. > > Sorry, lwbrx appeared there by mistake, don't consider it. > > A minor note so that it' clear which opcode values and masks I mean from > binutils point of view. Let's consider entry for lwbrx. > > https://git.libre-soc.org/?p=binutils-gdb.git;a=blob;f=opcodes/ppc-opc. > c#l8223 > > {"lwbrx", X(31,534), X_MASK, PPCCOM, 0, {RT, > RA0, RB}}, > > #define X(op, xop) (OP (op) | ((((uint64_t)(xop)) & 0x3ff) << 1)) > #define OP(x) ((((uint64_t)(x)) & 0x3f) << 26) > #define XRC(op, xop, rc) (X ((op), (xop)) | ((rc) & 1)) > #define X_MASK XRC (0x3f, 0x3ff, 1) > > With our code, what we get is: > Instruction(name='lwbrx', opcode=FieldsOpcode(value=0x7c00042d, > mask=0xfc0007ff)) Looks great (perfect). I have no problem at all understanding that 0xfc0007ff came from a *pair* of CSV files, or a hierarchy from power_decoder(). The major contributed to the fc the Minor 31 to the 7ff Most of binutils is quite obviously handwritten and given the number of instructions I consider that quite insane. the minor 31 CSV is NOT the same as how binutils does it. changing the CSV files absolutely cannot happen. Hang on... 733 Subdecoder(pattern=31, opcodes=get_csv("minor_31.csv"), 734. opint=True, bitsel=(1, 11), suffix=0b00101, subdecoders=[]), 0x7ff is off by one. It should be 0x7fe. bitsel is 1..11 so bits in mask must be 0b0000000...11111111110 111 1111 1110 is 7 f e not 7ff you may also find that 00042d has not been shifted up by one (start of bitsel) 42d binary 100 0010 1101 no needs shifting. Bit 0 should be 0
(In reply to Luke Kenneth Casson Leighton from comment #45) > 0x7ff is off by one. It should be 0x7fe. bitsel is 1..11 100 0010 110 is 534 decimal so no shift needed, the example you gave I guess had Rc=1 But the mask *is* incorrect unless you were definitely intending it to be "lwbrx." rather than lwbrx. Given that lwbrx. does not exist I suspect you meant 7c00042c
Yeah this was a huge mix-up. The code now produces Instruction(name='lwbrx', opcode=FieldsOpcode(value=0x7c00042c, mask=0xfc0007fe)). However, that said, the _binutils_ mask should have been be 0xfc0007ff, because X_MASK includes Rc field. Unlike our mask, the binutils mask is just the way to describe which fields are not expected to be operands. Here's the documentation: /* The opcode mask. This is used by the disassembler. This is a mask containing ones indicating those bits which must match the opcode field, and zeroes indicating those bits which need not match (and are presumably filled in by operands). */
(In reply to Dmitry Selyutin from comment #47) > Yeah this was a huge mix-up. The code now produces Instruction(name='lwbrx', > opcode=FieldsOpcode(value=0x7c00042c, mask=0xfc0007fe)). Hoorah. > However, that said, > the _binutils_ mask should have been be 0xfc0007ff, because X_MASK includes > Rc field. ok see below. > unlike our mask, the binutils mask is just the way to describe > which fields are not expected to be operands. Yes. And they construct that by hand rather than autogenerate . right. Deep breath. Paul Anton and the others at IBM Research who wrote microwatt kinda know power ISA the *real* Power ISA which is actually implemented by IBM POWER8 9 10 as opposed to what went in the spec if you know what I mean. One of the very annoying things that was done in the spec was, you can have bits that are ignored rather than have to be set to RESERVED values. Divide returns zero on overflow by POWER9 but the spec says "UNDEFINED" for example. Also mulhw if you look at the opcode there is *room* for an OE bit but rather than throw an exception POWER9 will blithely IGNORE that bit and execute the instruction as if it had not been set. There is a whole list of those in DecodeOE in power_decoder2.py, sigh. Likewise in the case of lwbrx, you *can* actually set bit 31 to 1 or 0 and IBM POWER9 hardware will *not care* you should be getting a sinking feeling or at least face-palm moment, but also understanding and appreciating that because Power ISA decode is so complex they *had* to take these shortcuts. Normally a spec makes these things RESERVED, basica!ly, so an illegal instruction trap should be thrown, but it is too costly in gates. Point is, how binutils does things is perfectly legal but misses instructions that should *not be attempted anyway* (bit 31=1) and *our* decode will get *both* the decodes, the one that is normally done (0x7c00042c) *and* also the one that strictly speaking you should not create (0x7c00042d) but if you did *IBM hardware would actually execute it*. sigh :)
Hooray, here's what my local binutils version produces: { .name = "sv.lwbrx", .desc = { .opcode = { .value = 0x7c00042c, .mask = 0xfc0007fe, }, .in1 = SVP64_IN1_SEL_RA_OR_ZERO, .in2 = SVP64_IN2_SEL_RB, .in3 = SVP64_IN3_SEL_NONE, .out = SVP64_OUT_SEL_RT, .out2 = SVP64_OUT_SEL_NONE, .cr_in = SVP64_CR_IN_SEL_NONE, .cr_out = SVP64_CR_OUT_SEL_NONE, .sv_ptype = SVP64_PTYPE_P2, .sv_etype = SVP64_ETYPE_EXTRA2, .sv_in1 = SVP64_EXTRA_NONE, .sv_in2 = SVP64_EXTRA_IDX2, .sv_in3 = SVP64_EXTRA_NONE, .sv_out = SVP64_EXTRA_IDX0, .sv_out2 = SVP64_EXTRA_NONE, .sv_cr_in = SVP64_EXTRA_NONE, .sv_cr_out = SVP64_EXTRA_NONE, }, }, Note that this already works with instruction database! I also was able to drop some duplicated code. I hope that in a couple of days I'll be able to check the Python code, fix the errors (including the one mentioned by Jacob) and re-generate the C code. After this, I'll add the disassembler code which uses the opcode.
The opcode should not be part of Desc, though, but rather a part of Record itself. struct svp64_desc { uint64_t in1 : 3; uint64_t in2 : 5; uint64_t in3 : 3; uint64_t out : 3; uint64_t out2 : 3; uint64_t cr_in : 4; uint64_t cr_out : 3; uint64_t sv_ptype : 2; uint64_t sv_etype : 2; uint64_t sv_in1 : 3; uint64_t sv_in2 : 3; uint64_t sv_in3 : 3; uint64_t sv_out : 3; uint64_t sv_out2 : 3; uint64_t sv_cr_in : 3; uint64_t sv_cr_out : 3; uint64_t : 15; }; struct svp64_opcode { uint32_t value; uint32_t mask; }; struct svp64_record { const char *name; struct svp64_opcode opcode; struct svp64_desc desc; }; { .name = "sv.lwbrx", .opcode = { .value = 0x7c00042c, .mask = 0xfc0007fe, }, .desc = { .in1 = SVP64_IN1_SEL_RA_OR_ZERO, .in2 = SVP64_IN2_SEL_RB, .in3 = SVP64_IN3_SEL_NONE, .out = SVP64_OUT_SEL_RT, .out2 = SVP64_OUT_SEL_NONE, .cr_in = SVP64_CR_IN_SEL_NONE, .cr_out = SVP64_CR_OUT_SEL_NONE, .sv_ptype = SVP64_PTYPE_P2, .sv_etype = SVP64_ETYPE_EXTRA2, .sv_in1 = SVP64_EXTRA_NONE, .sv_in2 = SVP64_EXTRA_IDX2, .sv_in3 = SVP64_EXTRA_NONE, .sv_out = SVP64_EXTRA_IDX0, .sv_out2 = SVP64_EXTRA_NONE, .sv_cr_in = SVP64_EXTRA_NONE, .sv_cr_out = SVP64_EXTRA_NONE, }, },
(In reply to Dmitry Selyutin from comment #49) > Hooray, here's what my local binutils version produces: > > { > .name = "sv.lwbrx", > .desc = { > .opcode = { > .value = 0x7c00042c, > .mask = 0xfc0007fe, awesome > Note that this already works with instruction database! briiilliant
also have this test program which confirms assembler-disassembler for a whole stack of instructions https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/sv/trans/test_pysvp64dis.py;hb=HEAD