hi michael, SigDecode has had these added to it: comb += self.x_s.eq(self.df.FormX.S[0]) comb += self.x_sh.eq(self.df.FormX.SH[0:-1]) comb += self.dq_xs_s.eq(self.df.FormDQ.SX_S[0:-1]) DecodeFields itself is where i was intending to be the canonical location for adding these kinds of things. if you feel that they should be added as signals, then we should do them in an automated fashion, because otherwise, after a hundred instructions are added, SigDecode will be jammed with almost 200 signal fields... *all of which were added by hand*.
Fixed in 1c9a58b
https://git.libre-riscv.org/?p=soc.git;a=summary remember ti git push it tho :)
what do you think of the idea of adding signals with the same name as the fields? btw i'm starting that mini-parser and will be looking to drop a dictionary of the name of the instruction's form (Form-X) into the exec context, to test them. so actually creating signals (again, automatically and/or programmatically) makes things slightly easier.
(In reply to Luke Kenneth Casson Leighton from comment #3) > what do you think of the idea of adding signals with the same name as > the fields? Haven't you already done this with power_fields/power_fieldsn?
(In reply to Michael Nolan from comment #4) > (In reply to Luke Kenneth Casson Leighton from comment #3) > > what do you think of the idea of adding signals with the same name as > > the fields? > > > Haven't you already done this with power_fields/power_fieldsn? if you recall i mentioned this a month ago particularly when we did those recursive expressions. the distinction between nmigen expressions and nmigen signals is very important. they are expressions rather than signals which, if you recall, results in duplication of those expressions wherever they are used (hence the massivr graphs ladt month despite the code only being a few lines long) if the expressions are used a hundred times then they are copied into the HDL a hindred times because they are a nmigen AST tree fragment. as signals, the *signal* would be copied and would show up in the yosys graphs.
(In reply to Luke Kenneth Casson Leighton from comment #5) > they are expressions rather than signals > which, if you recall, results in duplication of those expressions wherever > they are used (hence the massivr graphs ladt month despite the code only > being a few lines long) > Ah, I see now
Ok, I've got it sort of working in df295b5. I removed TopPowerDecoder's inheritance of DecodeFields and moved DecodeFields to a member. I then had TopPowerDecoder go through each of the "common" fields in DecodeFields and copy it to a signal. This works, but it doesn't take care of the form specific fields (e.g. dec.FormXL.XO[9]), and I'm not sure what to do with those. I suppose I could add in submodules for each of the forms and copy their signals like I did with the common ones, would that work for you?
(In reply to Michael Nolan from comment #7) > Ok, I've got it sort of working in df295b5. I removed TopPowerDecoder's > inheritance of DecodeFields and moved DecodeFields to a member. I then had > TopPowerDecoder go through each of the "common" fields in DecodeFields and > copy it to a signal. great. > This works, but it doesn't take care of the form > specific fields (e.g. dec.FormXL.XO[9]), and I'm not sure what to do with > those. I suppose I could add in submodules for each of the forms and copy > their signals like I did with the common ones, would that work for you? submodules is unnecessary (and a bit overkill): an object that is a dummy class would do (similar to the use of NamedTuple) class Form: pass or, actually, just a NamedTuple again. hmm take a look see what you think commit 5374cb82b _should_ be accessible as comb += something.eq(decoder.FormX.RA) which becomes important in the autogenerator code because each instruction is listed as having a "Form", and we can drop the decoder.Form{something} into its context, and the pseudo-code fragment will go "oh, i recognise these variables RA, RT etc. as being from Form{something}"
Interesting, it looks like we're going to need to remove the [0:-1] from anything that uses the fields now.
(In reply to Michael Nolan from comment #9) > Interesting, it looks like we're going to need to remove the [0:-1] from > anything that uses the fields now. Oh right, that removes the last value from the signal normally. Anyways, all the tests pass now
(In reply to Michael Nolan from comment #9) > Interesting, it looks like we're going to need to remove the [0:-1] from > anything that uses the fields now. it was a bad hack, i couldn't work out how to just refer to the field without going through __getattr__. now that everything goes *through* the equivalent of that, and is now sub-signals (slices of the original opcode), yes that hack should not be necessary... *as long* as we refer to those fields.
(In reply to Luke Kenneth Casson Leighton from comment #11) > be necessary... *as long* as we refer to those fields. correction: sub-signals.