https://libre-soc.org/irclog/%23libre-soc.2021-09-07.log.html#t2021-09-07T00:17:54
the basic idea here is to abstract out state testing of any simulator or executor of Power ISA instructions, to be able to make an exact comparison of ANY two such simulators/executers. the things to test (extract from two sim/exers and compare): 1) ALL registers. GPR, FPR, SPR, MSR, PC, CR 2) ALL memory locations (with the option to specify some) on the list of things that provide state needing test/compare: 1) qemu via pygdbmi 2) power-gem5 via same 3) cavatools-power (when it is ready) via same 4) **DIRECT** gdb native execution (also via pygdbmi) on e.g. IBM POWER9 5) ISACaller simulator (from openpower-isa) 6) LibreSOC HDL (via nmigen simulation) 7) microwatt HDL (most likely via cocotb using GHDL) all of these therefore need a CONSISTENT defined way to "extract": 1) all registers 2) memory at specific address locations this is NOT about creating *ONE* API in *ONE* class. it is about creating *TWO* APIs that are directly and inextricably linked. the tester API *USES* the testee (state) API to obtain the full list of registers (and memory values) from *two* testees and once obtained then and only then can the tester actually make the comparison.
kyle, so that this is clear to you, can you first take a look at test_core.py check_regs() and split it into THREE functions: 1) a function that extracts registers from the HDL core 2) a function that extracts registers from the sim object 3) a function that compares the two. example: 147 def check_regs(dut, sim, core, test, code): 148 # int regs 149 intregs = [] 150 for i in range(32): 151 if core.regs.int.unary: 152 rval = yield core.regs.int.regs[i].reg 153 else: 154 rval = yield core.regs.int.memory._array[i] 155 intregs.append(rval) 156 print("int regs", list(map(hex, intregs))) 157 for i in range(32): 158 simregval = sim.gpr[i].asint() 159 dut.assertEqual(simregval, intregs[i], 160 "int reg %d not equal %s. got %x expected %x" % \ 161 (i, repr(code), simregval, intregs[i])) becomes: 147 def get_core_hdl_regs(dut, sim, core, test, code): 148 # int regs 149 intregs = [] 150 for i in range(32): 151 if core.regs.int.unary: 152 rval = yield core.regs.int.regs[i].reg 153 else: 154 rval = yield core.regs.int.memory._array[i] 155 intregs.append(rval) ... return intregs and: ... def get_sim_regs() 157 for i in range(32): 158 simregval = sim.gpr[i].asint() ... simregs.append(...) ... return simregs and: def check_regs() get_sim_regs() get_core_regs() for i, (regsim, regcore) in enumerate(zip (simtegs, coreregs): dut.assertEqual(etc etc) it should be pretty obvious where that leads: a class is needed which "collects" GPRs, CRs, etc etc etc etc. rather than lists, a class *containing* lists.
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=885c9aa6fd7871d12eafd0a8e4a023eff5843ff0 i've added in some TODO comments which show what needs doing, but *work incrementally* towards the goal: don't do absolutely everything at once. suggest creating a class which takes core as a constructor argument: class CoreTestingThing: def __init__(self, core): self.core = core and add a function get_gpr_regs(): def get_gpr_regs(self): self.intregs = [] blah blah the reason is because "yield from" will otherwise get thooorrroughly in the way. i suggest starting with the *simulator* variant of that, first.
*** Bug 370 has been marked as a duplicate of this bug. ***
Start of breaking out functionality of check_regs https://git.libre-soc.org/?p=soc.git;a=commit;h=dff1ed6ba3b97ab23d67107d35574163f079793c
(In reply to klehman9 from comment #5) > Start of breaking out functionality of check_regs > > https://git.libre-soc.org/?p=soc.git;a=commit; > h=dff1ed6ba3b97ab23d67107d35574163f079793c ok so on the face of it, this looks great. however, digging a little deeper: 1) generally, the functions that are called are placed _before_ those that call them. also, PEP8 (which we follow, strictly), you put 2 blank lines between functions and classes, and one space between function arguments. i've done the moving of functions, i leave you to sort this out: def compare_core_sim_regs(dut,regsim,regcore, code): ^space ^space 2) remember i said, "only do the simulation part first"? this is because the HDL part uses "yield", and when using "yield" you have to *chain* sub-functions by also doing "yield from". that means you can't also use them to return a parameter (in this case, the list) 3) remember, also, i said, "make sure to run unit tests *before* committing". i did that, and got: ERROR: run_all (soc.simple.test.test_runner.TestRunner) [case_0_regression_unconditional] ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/lkcl/src/libresoc/soc/src/soc/simple/test/test_runner.py", line 298, in process yield from check_regs(self, sim, core, test, code) File "/home/lkcl/src/libresoc/soc/src/soc/simple/test/test_core.py", line 156, in check_regs compare_core_sim_regs(dut,simregs,intregs,code) File "/home/lkcl/src/libresoc/soc/src/soc/simple/test/test_core.py", line 237, in compare_core_sim_regs (i, repr(code), regsim, regcore)) TypeError: %x format: an integer is required, not Signal which means that literally every single unit test for HDL is now broken. hooray! :) so, given quite how obtuse yield is, i've sorted it with this: commit 4d76cc90de0dc9d97cd6f00a175194bccdde62a1 (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Tue Sep 7 22:37:06 2021 +0100 fun fixing of get_core_hdl_regs, "yield from" take a look and see what i did, there. it's not the best solution, but we can work incrementally. i'll run the full test_issuer.py, to make sure it's all good. next, i leave it up to you if you want to proceed to making a couple of classes which store intregs, or whether you want to proceed to doing CRs in the same style: one function for HDL-crs, one function for SIM-crs if you make a pair of classes which store intregs, it should be obvious that: 1) they should both store intregs NOT one stores intregs the other stores simregs 2) they should both have the exact same function name get_intregs NOT one has a function named get_core_hdl_regs the other called get_sim_regs 3) the name of each class should be HDLState and SIMState or something that reflects the fact that one is HDL-related and the other is Sim-related start to see how it pieces together? one incremental step at a time.
also, each class should not RETURN intregs/simregs, it should STORE intregs/simregs. this helps us get around the "yield" problem.
Yeah, coming together with shall we say a good sized misstep. Need the pep8 checker to become habit forming. Shortest test to run to verify?
autopep8 is functional but you have to be careful with it. typically i hand-edit test_issuer.py and comment out all but one of the lines. yes i am keenly aware that is lame. try not to commit modified test_issuer.py like wot i have done maaany times by mistake. the LD/ST one is pretty short. yes i have been meaning to add getopt parsing or any kind of sys.argv parsing to test_issuer.py for at least a year. one that is quite short is test_issuer_svp64.py runs in a couple of minutes.
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=5eb5477aef6abae55dde17cc5d5f182b32bda10b modified test_core (not pushed) svp64 passes with class
(In reply to klehman9 from comment #10) > https://git.libre-soc.org/?p=soc.git;a=commitdiff; > h=5eb5477aef6abae55dde17cc5d5f182b32bda10b > > modified test_core (not pushed) svp64 passes with class ah brilliant, that looks perfect. with an HDL class that has identical named get_intregs, the API begins to take shape in an "emergent" fashion, all the while keeping the code running and functional. it should be clear that for those "expected" results back in test_caller_shift_rot.py a class instance with self.intregs statically created could *also* be passed in as a *third* "thing to compare against".
(In reply to klehman9 from comment #10) > https://git.libre-soc.org/?p=soc.git;a=commitdiff; > h=5eb5477aef6abae55dde17cc5d5f182b32bda10b > > modified test_core (not pushed) svp64 passes with class the HDL version can be pretty much identical, only thing to remember is to call the get_intregs() function with "yield from" at the front, that's all. btw do feel free to likewise commit the HDL class version as an "unused thing", in exactly the same way. then you only have to be concerned about python syntax errors, no need to even run unit tests.
https://git.libre-soc.org/?p=soc.git;a=commit;h=aa95068eda3dcd508383dda33c0e389a2b718401 Rest of the sim. As for the HDL, spent quite a bit of time trying to get it to work. Perhaps I'm missing something obvious, but it doesn't like "yield from" within the get_intregs method. I'll commit the byzantine mess where it actually uses yield from and iterates...but basically returns no data into the list.
https://git.libre-soc.org/?p=soc.git;a=commit;h=53763d71664e428f8cacd792e58a0f8ecafb1909 Using yield from results in File "/home/klehman/src/soc/src/soc/simple/test/teststate.py", line 41, in get_intregs rval = yield self.core.regs.int.memory_array[i] AttributeError: 'IntRegs' object has no attribute 'memory_array' caller portion: hdlregs = HDLState(core) yield from hdlregs.get_intregs()
(In reply to klehman9 from comment #13) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=aa95068eda3dcd508383dda33c0e389a2b718401 > > Rest of the sim. > > As for the HDL, spent quite a bit of time trying to get it to work. Perhaps > I'm missing something obvious, but it doesn't like "yield from" within the > get_intregs method. I'll commit the byzantine mess where it actually uses > yield from and iterates...but basically returns no data into the list. ah so this is one of the oddities of python yield. you don't use "yield from" *IN* the get_intregs method, you use "yield from" **ON** the get_intregs method. x = HDLState(args) yield from x.get_intregs() NOT class HDLState: def get_intregs(...) yield from xxx you'll need to look up a tutorial on python yield, but basically the above covers it. basically, "yield" will yield "the thing", and "yield from" will DRILL DOWN into the "thing", allowing *IT* to perform "yielding" of results. summary: you prefix a function with "yield from", you prefix the things you want to *be* yielded with *just* "yield"
https://git.libre-soc.org/?p=soc.git;a=commit;h=18aaa0d2de6945f93ff5bb80d3b64e8dce523e6c hdl portion finished and compared results in test_core
(In reply to klehman9 from comment #16) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=18aaa0d2de6945f93ff5bb80d3b64e8dce523e6c > > hdl portion finished and compared results in test_core that looks really good, it's starting to take shape. a quick tip: to turn any function into a generator, just add "yield" somewhere in it. first line will do. not "yield something", just one word, "yield". then, you can (actually, have to) then do "yield from sim.get_intregs()" *exactly* like with HDL. it's a bit of a kludge but the end-result is that the two functions have the exact same use-characteristics... and oh look, that's the definition of an API. class SimState: def get_intregs(srlf): yield # nothing self.intregs.append(...)
(In reply to Luke Kenneth Casson Leighton from comment #17) > that looks really good, it's starting to take shape. a quick tip: > to turn any function into a generator, just add "yield" somewhere > in it. first line will do. not "yield something", just one word, > "yield". umm, actually you want the `yield` somewhere where it won't be executed, because `yield` does actually yield None which is probably not desired: def yield_none(): yield # equivalent to `yield None` print(list(yield_none())) # prints `[None]` if you want a generator that doesn't yield anything, do like so: def yield_nothing(): if False: yield print(list(yield_nothing())) # prints `[]`
One other thing, I'd like to request that the test API be designed with parallelism in mind...python usually uses running separate processes with each process running some of the tests, see pytest-xdist's docs for an explanation. Running tests in parallel can make it >12x as fast for me to run tests (since I have a 12-core/24-thread cpu). For an example of making a test parallelizable, see: https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller_long.py;h=d136361260e596f737eb50e94953efb56c1d2a50;hb=49bf0a79fa16d2d524e3d1501858a42166ee6d61#l226 Note how there are 16 separate unittest test cases test_mul_pipe_3_arg_{0..15} that each run 1/16 of the test, which allows test runners like pytest with pytest-xdist to run each in parallel, saving a bunch of time.
https://git.libre-soc.org/?p=soc.git;a=commit;h=b5805e3280ed6a5ad4c86dcfb1b92f58a49cc0bf Finished generators/general cleanup
(In reply to Jacob Lifshay from comment #19) > One other thing, I'd like to request that the test API be designed with > parallelism in mind... ah this is an orthogonal issue. as in: it's a low-level comparator API that should have absolutely nothing to do with parallelism at the *unit test runner* level (be that nosetests or any other) bear in mind that attempting to run multiple parallel qemu cosimulations may simply not be practical. but even there this API has absolutely nothing to do with how it's used: it is PURELY a comparator API. > Note how there are 16 separate unittest test cases > test_mul_pipe_3_arg_{0..15} that each run 1/16 of the test, which allows > test runners like pytest with pytest-xdist to run each in parallel, saving a > bunch of time. it's a good design, yet at the same time has nothing to do with this bugreport, which is, solely about extracting and then comparing two (or more) sets of "state".
(In reply to Jacob Lifshay from comment #18) > if you want a generator that doesn't yield anything, do like so: > def yield_nothing(): > if False: > yield good catch. (In reply to klehman9 from comment #20) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=b5805e3280ed6a5ad4c86dcfb1b92f58a49cc0bf > > Finished generators/general cleanup nicely done. if you are now using it, and have run a couple of tests, do commit it. i will pick it up and run bigger ones on my beefy machine. if you're not quite confident about it then commit as an alternative function name that's a one-line change for me to actually use it.
(In reply to Luke Kenneth Casson Leighton from comment #21) > (In reply to Jacob Lifshay from comment #19) > > One other thing, I'd like to request that the test API be designed with > > parallelism in mind... > > ah this is an orthogonal issue. as in: it's a low-level comparator > API that should have absolutely nothing to do with parallelism at > the *unit test runner* level (be that nosetests or any other) parallelism has to be accounted for, otherwise you could do something like write to /tmp/program.s and expect it to not be modified -- that won't work when running tests in parallel since tests would overwrite files used by other tests (we already have that problem with vcd and gtkw files, luckily the tests don't need to read the files back in, otherwise they'd fail).
(In reply to Jacob Lifshay from comment #23) > > parallelism has to be accounted for, otherwise you could do something like > write to /tmp/program.s and expect it to not be modified -- that won't work > when running tests in parallel since tests would overwrite files used by > other tests (we already have that problem with vcd and gtkw files, luckily > the tests don't need to read the files back in, otherwise they'd fail). again, to reiterate: that has nothing to do with *this* API, whose sole exclusive one simple and ONLY purpose is: 1) to extract state 2) to compare state. the issue that you raise is relevant... it is juat not relevant to *this* bugreport, and therefore should be discussed elsewhere so as not to cause a distraction to a new contributor.
https://git.libre-soc.org/?p=soc.git;a=commit;h=79e28bfdc43c60de661599a02726af245aaa74d3 blocked comments to be removed. One thing I think would be beneficial is adding in a static class with register names. Given everything from the state class can be accessed as a list, enumerating with register name lists would allow just one generic compare function for every type of register while being descriptive. Would only need to add an additional argument to compare function.
(In reply to klehman9 from comment #25) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=79e28bfdc43c60de661599a02726af245aaa74d3 > > blocked comments to be removed. great. i'll run it shortly. ok the pep8 whitespace mods, i have difficulty discerning them from functional-related changes. do keep to "one purpose, one commit" (see HDL_workflow). i read this in the diff, and wondered, "what functional change is it making? ah it's just whitespace" def setup_regs(pdecode2, core, test): @@ -71,7 +72,7 @@ def setup_regs(pdecode2, core, test): print("setup cr reg", hex(cr)) for i in range(8): #j = 7-i - cri = (cr >> (i*4)) & 0xf + cri = (cr >> (i * 4)) & 0xf #cri = int('{:04b}'.format(cri)[::-1], 2) print("setup cr reg", hex(cri), i, crregs.regs[i].reg.shape()) @@ -121,7 +122,7 @@ def setup_regs(pdecode2, core, test): if sprname == x.name: print("setting slow SPR %d (%s) to %x" % (i, sprname, val)) - if not sprname in mmu_sprs: + if sprname not in mmu_sprs: yield sregs.memory._array[i].eq(val) else: yield from set_mmu_spr(sprname, i, val, core) @@ -148,6 +149,7 @@ def setup_regs(pdecode2, core, test): > One thing I think would be beneficial is adding in a static class with > register names. Given everything from the state class can be accessed as a > list, enumerating with register name lists would allow just one generic > compare function for every type of register while being descriptive. Would > only need to add an additional argument to compare function. mmm, yeah, that'd work. openpower/consts.py would be a good place to include them: do (as a separate single commit) put explanatory comments, openpower-isa is a reference as much as anything.
ok, right, so are you thinking along the lines of (similar to how nmigen Record works, and also PowerDecoder RowSubset): * a list of regnames * a base class (inherited by both HDLState and SimState) * a function in the base class which takes the regnames and uses getattr to access the "reg to be compared" something like that? do bear in mind a couple of things: 1) the existence of some regs (e.g. Floating Point) is conditional. both the HDL and simulator this is optional 2) the *number* of regs can vary. Power ISA v3.0B has only 32 GPRs and 32 FPRs, but SVP64 has 128 for both *and* will have *16* 32-bit CRs. 3) SPRs (which you can pick up a full list *already* from the power_enums.py class are so numerous we had to dynamically allow only a subset of them, as determined at runtime. so you have to bear in mind, the total numbers and size may vary, in the final version, and so has to be adaptive.
Spacing noted. Naming - Yeah, something like that. I'll keep the potential naming thing all in mind for the future. Definitely fine as is for the moment.
(In reply to klehman9 from comment #28) > Spacing noted. > > Naming - Yeah, something like that. I'll keep the potential naming thing > all in mind for the future. Definitely fine as is for the moment. ok cool. test_issuer.py passes btw so nothing broken, a good sign. what's next... the next code-morph would be one that reduces the amount of explicit function calls and those that are identical. i.e.: * group all sim.get_xxx() together * group all hdl.get_xxx() together * make a base class containing a function that calls self.get_xxx() and work towards a function (which check_regs would call) that takes: * a dut * a dictionary {'sim': sim, 'hdl': core} * anything else and does a lookup in a "Factory" dict: klsfactory = {'sim': SimState, 'hdl': HDLState } gets the class to create, passing sim or core in. you get the general idea
https://git.libre-soc.org/?p=soc.git;a=commit;h=06b8931eb167030bc47a287424ce4b9cadd854ce Hopefully this is what you were looking in regards to the base class. Has no effect on unmodified test caller, svp passes. Svp passes when changes are made to utilize new get_state method.
(In reply to klehman9 from comment #30) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=06b8931eb167030bc47a287424ce4b9cadd854ce > > Hopefully this is what you were looking in regards to the base class. pretty much, yeah! creating the functions with "pass" in them is unnecessary btw, one of the weirdities of python, you can have "APIs as conventions" and follow the Liskov Substitution Principle rather than have explicit functions. > Has no effect on unmodified test caller, svp passes. Svp passes when > changes are made to utilize new get_state method. great! getting there. so what do you think of the idea of adding the check_regs functions to the new class? or, do you think it would be better to have a global function in teststate.py which takes: 1) dut parameter 2) test fn name parameter 3) code parameter 4) *list* of state(s) to compare where it would test el0 against el1, el1 against el2, el2 against el3 .... or... both?
Good question. I'm an encapsulation type guy. With that said if made global, the class version of it would be basically free consisting of a line or two unless I'm oversimplifying things. And would give a syntax choice as a bonus.
(In reply to klehman9 from comment #32) > Good question. > > I'm an encapsulation type guy. With that said if made global, the class > version of it would be basically free consisting of a line or two unless I'm > oversimplifying things. And would give a syntax choice as a bonus. i was kinda thinking along the lines of __add__ and operator.add combined with "reduce". in other words * a class member function (equivalent to __add__) which takes one other state that checks against another state (hell, it could even *be* __eq__ but we need some debug info raised, about where the error occurred, hmm, well there's nothing wrong with splitting out the comparison from the reporting i.e. calling dut.assertTrue(wasiteq) instead) * a function which *calls* the class member function or just does for i in range(len(list)-1): testme = list[i] == list[i+1] dut.assertTrue(testme, a_report) hmm, maybe not, because the report needs to include exactly which reg and what itvwas compared against. and, ah, the name of its type (back in the original dict, "sim" or "hdl" or "microwatt" or "qemu")
https://git.libre-soc.org/?p=soc.git;a=commit;h=b9450a8d71b2fa1d4d01cfe9063436681b5f79a6 Gets state loaded when initiated. tested, passes in test core using: testdic = {'sim': sim, 'hdl': core} simstate = yield from TestState("sim",dut,testdic) corestate = yield from TestState("hdl",dut,testdic)
https://git.libre-soc.org/?p=soc.git;a=commit;h=50a8c2147c00271bc82cc3298290d7b046d6f3ae Added compare function as a first step (besides, it needed to come over in some fashion regardless). Tested fine. Asserts now show what type/class of the different states are involved.
(In reply to klehman9 from comment #35) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=50a8c2147c00271bc82cc3298290d7b046d6f3ae > > Added compare function as a first step (besides, it needed to come over in > some fashion regardless). > > Tested fine. Asserts now show what type/class of the different states are > involved. excellent, do drop in its usage as well. i'll run the long test(s) next on the TODO: given that this is being called tens of thousands of times it's probably a good idea to begin to add some docstrings :) also, just occurred to me: the Factory needs "outside modules" to be capable of adding *themselves* to the dictionary. example: this API (teststate.py) is one that i'd actually recommend moving to openpower-isa (because it's fundamentally part of the rest of the test infrastructure rather than unique to the soc repo)... ... but HDLState has to *stay* in soc because it's got nothing to do with the *rest* of the openpower-isa repo. (SimState on the other hand *could* move to openpower-isa) thus, the Factory dictionary is one of the rare instances in python where it should be a global, and a global function made available to add things to it. that definitely needs comments and a docstring to explain! then, when qemustate.py is created, it calls the factory-function to add QemuState (again, comments/docstrings explaining that this is unusual) rather than having files accumulate more and more dependence. ultimately, a third party should be able to create a completely independent project and add themselves to the list of things to test against... *WITHOUT* needing to demand write-access to the libre-soc source code in order to do so, and *WITHOUT* needing to create a forked copy of it, as a poor substitute.
https://git.libre-soc.org/?p=soc.git;a=commit;h=b6c47e36d71e09a98ae8deac6f9c7ba5a884e1ae modified test_core now live
commit 86f52a45b43bfc596ebf7217cbb1cc2ce0354204 (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Sun Sep 12 14:21:23 2021 +0100 create new function teststate_check_regs which is called by check_regs teststate_checkregs does not care how many pieces of state it is asked to compare. could be 2, could be 3, could be 30 https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=86f52a45b43bfc596ebf7217cbb1cc2ce0354204 so that allows an arbitrary number of things to be compared.
https://git.libre-soc.org/?p=soc.git;a=commit;h=8e4d86cf105f8ab790982c87c65bc52ea228fd90 Debating on best final resting place for the split. Been looking into qemu and what is already there.
(In reply to klehman9 from comment #39) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=8e4d86cf105f8ab790982c87c65bc52ea228fd90 > > Debating on best final resting place for the split. dropped into openpower-isa openpower/test/stste.py > Been looking into qemu and what is already there. an "ExpectedState" is likely a simple thing to work on whilst investigating that. although, just call it from _check_regs() so, in e.g. shiftrot test: * create ExpectedState class (in new state.py file) * establish an instance of ExpectedState with int regs equal to the things that are being done manually with self.AssertEqual right now * call the check function at the end of the simulation the two things to pass in, obviously: sim and expected qemu is dog slow, this i suspect is a bug in pygdbmi
one thing to think about, if there is an expected state, where do we get the expected results? I think it might be better to just run multiple simulators/VMs and compare their results instead of isolating one of them as the "canonical expected result". building a custom result generator just to get the "expected result" can be alternatively seen as building a custom simulator to be used just for that specific test case, so I think it would be better to acknowledge that that is the case and that it is just like the other simulators, so should also be a simulator.
(In reply to Jacob Lifshay from comment #41) > one thing to think about, if there is an expected state, where do we get the > expected results? either by manual inspection or by running an equivalent function in python that is known-good (and also verified). there are about.... 100 such tests already, and many more are needed. in the case of the Power ISA Conformance Documents we *have* to assume the values listed are "correct". and be able to enter them. an ExpectedState allows that to be formally specified, where at the moment it is nonstandard and a bit of a mess. > I think it might be better to just run multiple > simulators/VMs and compare their results instead of isolating one of them as > the "canonical expected result". the idea is to do both. 1) compare simulators against simulators. 2) compare simulators against "expected results". > building a custom result generator just to get the "expected result" can be > alternatively seen as building a custom simulator to be used just for that > specific test case, so I think it would be better to acknowledge that that > is the case and that it is just like the other simulators, so should also be > a simulator. one immediate counterexample which shows this line of reasoning to be invalid is the OpenPOWER Foundation Compliance Test documents. we are *required* to extract the listed values and *required* to demonstrate that they have been run. the easiest way to do that is to put them into "ExpectedState". additionally it is not a problem at all to have unit tests that check simulators. the unit tests do not have to be sophisticated, they definitely do not need to be a full simulator. have you looked at any of the test_caller*.py unit tests? the SVP64 ones have been absolutely essential and serve a secondary purpose of acting as both examples and documentation. additionally if we find flaws it is very easy to report "Expected Results" by.pointing at the unit test, which will hsve a class instance, "ExpectedState" thus i count approximately four possibly five separate and distinct very good reasons why we need an ExpectedState. this is why i have been asking you to complete the BCD test in python (and only python) it is *not* a waste of time or waste of effort.
(In reply to Luke Kenneth Casson Leighton from comment #42) > (In reply to Jacob Lifshay from comment #41) > > one thing to think about, if there is an expected state, where do we get the > > expected results? > > either by manual inspection or by running an equivalent > function in python that is known-good (and also verified). > > there are about.... 100 such tests already, and many more > are needed. > > in the case of the Power ISA Conformance Documents we *have* > to assume the values listed are "correct". > > and be able to enter them. an ExpectedState allows that to > be formally specified, where at the moment it is nonstandard > and a bit of a mess. > > > I think it might be better to just run multiple > > simulators/VMs and compare their results instead of isolating one of them as > > the "canonical expected result". > > the idea is to do both. > > 1) compare simulators against simulators. > 2) compare simulators against "expected results". I'm saying expected results are a kind of simulator and should be treated as such. > > > building a custom result generator just to get the "expected result" can be > > alternatively seen as building a custom simulator to be used just for that > > specific test case, so I think it would be better to acknowledge that that > > is the case and that it is just like the other simulators, so should also be > > a simulator. > > one immediate counterexample which shows this line of reasoning to be > invalid is the > OpenPOWER Foundation Compliance Test documents. > > we are *required* to extract the listed values and *required* to demonstrate > that they have been run. easy (adjust API to match real tester API): # tester library class class ExplicitCaseSimulator(Simulator): def __init__(self, cases, make_key): self.cases = cases self.make_key = make_key def simulate(self, regs): key = self.make_key(regs) if key in self.cases: for k, v in self.cases[key].items(): setattr(regs, k, v) return regs raise NoResults() # causes tester to skip this simulator # in unit test we have the list of explicit test cases: cases = { # xor for demo purposes (1, 2): {"r1": 3}, (0x123, 0x456): {"r1": 0x123 ^ 0x456} } tester.addSimulator(ExplicitCaseSimulator(cases, lambda regs: (regs.r2, regs.r3)))
(In reply to Jacob Lifshay from comment #43) > (In reply to Luke Kenneth Casson Leighton from comment #42) > > (In reply to Jacob Lifshay from comment #41) > > > one thing to think about, if there is an expected state, where do we get the > > > expected results? > > > > either by manual inspection or by running an equivalent > > function in python that is known-good (and also verified). > > > > there are about.... 100 such tests already, and many more > > are needed. > > > > in the case of the Power ISA Conformance Documents we *have* > > to assume the values listed are "correct". > > > > and be able to enter them. an ExpectedState allows that to > > be formally specified, where at the moment it is nonstandard > > and a bit of a mess. > > > > > I think it might be better to just run multiple > > > simulators/VMs and compare their results instead of isolating one of them as > > > the "canonical expected result". > > > > the idea is to do both. > > > > 1) compare simulators against simulators. > > 2) compare simulators against "expected results". > > I'm saying expected results are a kind of simulator and should be treated as > such. none of these can in any way be called "a simulator" https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/test_caller.py;hb=HEAD even the DCT / FFT ones cannot in any way be called "a simulator". https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/test_caller_svp64_fft.py;hb=HEAD i used an external (excellent, well-written) library from the Nayuki Project which *itself* had unit tests that trace back to the original (very slow) DCT algorithm. > easy (adjust API to match real tester API): > # tester library class > class ExplicitCaseSimulator(Simulator): that's exactly what the ExpectedState class is designed to hold. expected results. > # in unit test we have the list of explicit test cases: > cases = { # xor for demo purposes > (1, 2): {"r1": 3}, > (0x123, 0x456): {"r1": 0x123 ^ 0x456} > } yes. that is exactly and precisely what i said. calculate or type the expected results, put them in an object, hand them to the check_regs function. done. calling it a "Simulator" is not useful or helpful when the API is in no way intended for simulating. it gets data *FROM* simulators but is not *itself* a simulator. ExpectedState is intended to be STATIC. you call it ONCE at the *END* of a *COMPLETED* simulation run. what you propose is massively more complex and requires actually understanding instructions, decoding them, maintaining a PC, updating of memory, it's huge and counterproductive. the purpose of ExpectedState is to be abundantly clear, brutally minimalist, and in no way complex in any way.
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=96d9e1f949c6b6d0a9a9cee75075cf0361853d67 Added basic expected class
(In reply to klehman9 from comment #45) > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=96d9e1f949c6b6d0a9a9cee75075cf0361853d67 > > Added basic expected class e = yield from ExpectedState() ah no. just e = ExpectedState(int_regs = [list, of, regs], pc = NNNN, cr = [list, of, CR, Fields] ) the calling of yield is *not* the responsibility of the unit test. the *check_regs* function has that responsibility. however for the get_intregs() etc. of ExpectedState they should be entirely blank. everything created from ExpectedState()'s *constructor*. real brutally simple. if there was a way to get rid of get_intregs() etc. even needing to exist in ExpectedState i would recommend doing it.
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=768e5265a89d4d195549b1e14dd4c033b3403210 Changed over first couple of tests in test_caller_shift_rot to use expected state.
(In reply to klehman9 from comment #47) > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=768e5265a89d4d195549b1e14dd4c033b3403210 > > Changed over first couple of tests in test_caller_shift_rot to use expected > state. oh, nice! yeah i like the idea of declaring a "blank" ExpectedState, then modifying it afterwards, explicitly. couple of suggestions: 1. + def check_regs(self, sim, e): + # int regs + for i in range(32): + self.assertEqual(sim.gpr(i), SelectableInt(e.intregs[i], 64), + "int reg %d -> sim not equal to expected." % (i)) this can be entirely replaced by.... ta-daaa.... the call to TestState() check_regs()! it is, after all, if you look closely, exactly the same code. 2. name the args (use kwargs) e = ExpectedState(pc=4) and put defaults of 0 for anything integers (pc) and None for lists. then, in the constructor, do this: if int_regs is None: int_regs = 32 if isinstance(int_regs, int): int_regs = [0] * int_regs what that does is allow us to: a) pass in a list of expected regs b) pass in the number of regs to be zero'd c) default to 32 zero'd regs. i assume you know NOT to put lists (any mutable object) as a default parameter to a function! because doing so is a SINGLETON pattern, believe it or not. this results in really hard to find bugs.
https://libre-soc.org/irclog/%23libre-soc.2021-09-16.log.html#t2021-09-16T17:00:38 commit 5018bfaed0328c4606f42a9feaccd58cb9fa731f (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Thu Sep 16 17:06:27 2021 +0100 moving teststate_check_regs written by klehman into openpower-isa ok so you *should* now just be able to create a function which is identical to the one in test_core.py def check_regs(dut, sim, e, test, code): # create the two states and compare testdic = {'sim': sim, 'expected': e} yield from teststate_check_regs(dut, testdic, test, code) we can then look at moving that function into a Base class (one derived from FHDLTestCase) so that it doesn't get duplicated 100s of times. one step at a time though
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=950ec659d34059398f19f4fb61524265fbba7bff defaults added to expected state
https://libre-soc.org/irclog/%23libre-soc.2021-09-17.log.html#t2021-09-17T15:19:31 > kylel so basically add an optional expected for add_case and then in check_regs choose the proper dict? basically, yes. the minor complication (not really) is that add_case stores everything in a class, actually a list of class instances, which the run_tst() picks up, enumerates, and runs as subTests on just the one Simulator/HDL-runner (we did this because it's so damn expensive to set up) thus the ExpectedState() will need to be added as an optional argument to test_data, in soc simple/test/test_runner.py that will be... TestAccumulatorBase and TestCase. https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/test/common.py;h=2bc0c1fab494720f388b24e2be598883381c535c;hb=HEAD#l117 * TestCase will need a new (optional) expected argument * TestAccumulatorBase.add_case will need the same * soc's simple/test/test_runner.py will need to *use* that BUT... BUTBUTBUT... note that just like back in test_caller_shiftrot.py, those Expected Results are **NOT** - cannot - be compared in the *MIDDLE* of the test run. they can only be called *AT THE END*. i will update the source code with the exact point where that can be done, with a TODO comment
https://libre-soc.org/irclog/%23libre-soc.2021-09-17.log.html#t2021-09-17T19:22:43 khlehman i was just about to try splitting out result-generation in test_core.py into a list-of-states for HDL and a list-of-states for ISACaller sim, and realised that memory-checking hasn't been added yet. # Memory check yield from check_sim_memory(self, l0, sim, code) could you tackle that next? check_sim_memory, in soc's test_compunit.py, is brain-dead-simple, 5 lines of code, but needs splitting out into 3 separate loops: * one to obtain the memory from the HDL "mem" object * one to obtain the memory from the simulator * one to compare the results now... the minor complication is, that if split, you don't necessarily know the size of the simulator memory (you'll see by examining the check_sim_memory function). have a hunt around (in decoder/isa/mem.py as well) see if you can think of a solution/workaround
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=82545236fe16a9947d8871848cda26f38c106ab5 https://git.libre-soc.org/?p=soc.git;a=commit;h=9bfa69243b8d2d7f9bcef284e3387c81f935ac70 Added compare_mem as a separate function...felt it safer that way for now but could be tossed into compare I suppose. As for the compare, on the sim side memory is basically composed of a list. So went with that method creating that out for the hdl side. Because of this, if a location is zero it will be skipped. Of course conceivably the sim side could store a zero to a location with a write, and this action would be disregarded in the compare operation. Not ideal, but for now a good start.
(In reply to klehman9 from comment #53) > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=82545236fe16a9947d8871848cda26f38c106ab5 > > https://git.libre-soc.org/?p=soc.git;a=commit; > h=9bfa69243b8d2d7f9bcef284e3387c81f935ac70 > > Added compare_mem as a separate function...felt it safer that way for now > but could be tossed into compare I suppose. yyeh, except it kinda feels better to do it explicitly, so it's clear what is being done. also... > As for the compare, on the sim side memory is basically composed of a list. > So went with that method creating that out for the hdl side. Because of > this, if a location is zero it will be skipped. Of course conceivably the > sim side could store a zero to a location with a write, and this action > would be disregarded in the compare operation. Not ideal, but for now a > good start. ... yeah nice. also, at some point, we may wish to make the API compare only those memory locations that were requested to be changed, by hooking into the mem read/write and observing the actual addresses requested. if done carefully it should avoid the problem that although instruction X requested ST to Y but actually wrote to location Z, and the comparison *completely missed it*. missing changes is why i added code that just blithely compares the entire memory (which is kept to only 64 bytes or so) nicely done.
Changes committed for using compare_mem in teststate in runner and core https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=9c4cbf72541f8f8ff7e9ec5191d1ed321a4f89d5 https://git.libre-soc.org/?p=soc.git;a=commit;h=07ab2ef32be27e32237b25d35a88f07e1b097a7a https://git.libre-soc.org/?p=soc.git;a=commit;h=a8371e5d5ba1b5179dc5a471b27fef7aec13fb29
(In reply to klehman9 from comment #55) > Changes committed for using compare_mem in teststate in runner and core > > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=9c4cbf72541f8f8ff7e9ec5191d1ed321a4f89d5 ah, since converting to a dict, it is important to also check the keys. or, hm, that those entries *not* in mem1 but are in mem2 are zero, and vice-versa. this may need the use of sets, to check. addrs1 = set(mem1.keys() addrs2 = same then there will be something in python sets where you can get "those things of set1 that are NOT in set2" and go from there.
https://www.programiz.com/python-programming/methods/set/difference found it. "difference"
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=b77c0164e6cc0d55af15da49d355b5addf9c3f05 Turned out to be a little logic puzzle to try to find the slickest way but difference made the difference.
(In reply to klehman9 from comment #58) > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=b77c0164e6cc0d55af15da49d355b5addf9c3f05 > > Turned out to be a little logic puzzle to try to find the slickest way but > difference made the difference. ha! neat! i like it. okaaay so with both mem and reg compare in place it should be possible to decouple the Sim from HDL, run them *separately* one after the other, collating a series of states in each case, and compare the states *afterwards*. this may waste time in some cases if one of them creates the wrong answer immediately, but hey. while i sort that out, can you do some basic (independent) unit tests, some fake mem and regs, just blast the values in explicitly to the HDLState and SimState, don't bother running a Sim or HDL loop at all, and check they are equivalent? the important ones are the @unittest.expectedFailure() when a mem location is either missing or different, and likewise a reg that is deliberately wrong.
got the setup of HDL memory to be independent of the Sim object. steps along the way to splitting out HDL-running from Sim-running commit a57d6918b4621a2deff7c6cab70a1828f25db56e (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Wed Sep 22 16:48:45 2021 +0100 split out function which processes initial test memory values in Mem class commit a575b24544aecbab9fbaa4e4ba5eb743ab4932c7 (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Wed Sep 22 16:56:09 2021 +0100 alter setup_tst_memory to take a test.mem rather than take a Sim object *containing* a Mem
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=f25faf58a3ed81573fc692780fa67df22b57f98f functional. TBD, split out the HDL-runner part, the Sim-runner part, and the compare-all-states part. it should now be blindingly obvious how to do the same thing for QemuState, and others.
(In reply to Luke Kenneth Casson Leighton from comment #51) > thus the ExpectedState() will need to be added as an optional > argument to test_data, in soc simple/test/test_runner.py > > that will be... TestAccumulatorBase and TestCase. > https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/test/ > common.py;h=2bc0c1fab494720f388b24e2be598883381c535c;hb=HEAD#l117 > > * TestCase will need a new (optional) expected argument did this one, and TestAccumulatorBase commit 7b3789085d19e0a8376afaf82dc225ae0abe5b88 (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Wed Sep 22 21:08:45 2021 +0100 add expected argument to TestCase commit 4dd9873c2edc42763996cc36408de3d8d2afc006 (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Wed Sep 22 21:16:44 2021 +0100 and add expected to TestAccumulatorBase
commit 1d1a018238c32c95f1625e955eda14b2a0c43e9c Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Wed Sep 22 21:35:14 2021 +0100 add test of expected results against last sim state
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=75300b05e802be0daaf7f9255a402bc9e6bc893a state class tests
(In reply to klehman9 from comment #64) > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=75300b05e802be0daaf7f9255a402bc9e6bc893a > > state class tests ha, that looks really good. gives confidence about catching errors. been running for 18 months, this code is so important for catching mistakes, changing it makes me nervous. i've a commit to make of an ALU test case ExpectedState, works great.
Yeah it's nice to have something uniform to do things with instead of a scattering of calls all over. I did get a chuckle with one of my failure tests didn't fail and it said unexpected success. I actually panicked a little thinking the mem checking wasn't totally correct but then realized i was calling compare and not compare_mem. Crisis overted.
commit f6b2ec6e8f0739fe9caf404094d692cdd69f5fb8 (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Thu Sep 23 23:40:04 2021 +0100 add option to run ISACaller Sim (or not) commit 9479cddb3533123d698e3d0fdf3e848e20c09ae2 Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Thu Sep 23 23:31:41 2021 +0100 add a new run_hdl parameter to TestRunner
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=7dfff09be53b97f9b9b14f79a38bcaa78aac43af should be clear what's needed. anything marked "if self.run_hdl" it goes into an HDLRunner. anything "if self.run_sim" into a SimRunner. the pspec object, pass it in as one of the arguments to the constructor of both the HDLRunner and SimRunner, they need to operate from the "same page" (the same context) also, you will have to pass in the Module instance (m) as an additional argument. go one step at a time, even just one function at a time (commit between each one)
https://git.libre-soc.org/?p=soc.git;a=commit;h=c5213803b82fd4d348c90fefff2a62eeb4c814d2 hdlrunner class with test_runner changes to use it
(In reply to klehman9 from comment #69) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=c5213803b82fd4d348c90fefff2a62eeb4c814d2 > > hdlrunner class with test_runner changes to use it that looks really good, next steps should be clear, moving the DMI calls behind a function, and so on. if you do HDL, i'll do Sim?
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=5bc2cf02f0df7639fbc6b2869b1d9cba71b9c8d2 moved the call to sim test-running behind the (evolving) SimRunner API
https://git.libre-soc.org/?p=soc.git;a=commit;h=f164672e4e03af1b63c51809f93bdc3d3be106ad https://git.libre-soc.org/?p=soc.git;a=commit;h=d5d62c713b127cd17a99ba124bd7320ab98f46c8 https://git.libre-soc.org/?p=soc.git;a=commit;h=10447554acce69654f27c0c30d8d19e92669095d wrapping (wrapped?) up changes over to HDLRunner
(In reply to klehman9 from comment #72) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=f164672e4e03af1b63c51809f93bdc3d3be106ad > > https://git.libre-soc.org/?p=soc.git;a=commit; > h=d5d62c713b127cd17a99ba124bd7320ab98f46c8 > > https://git.libre-soc.org/?p=soc.git;a=commit; > h=10447554acce69654f27c0c30d8d19e92669095d > > wrapping (wrapped?) up changes over to HDLRunner looks great - not quite. next phase once everything is "independent" (abstracted) is to remove all of the "if self.run_hdl" and "if self.run_sim" and to use those *once*... like this: def run_all(self): list_of_things_to_sim = [] if self.run_hdl: hdlrun = HDLRunner(....) list_of_things_to_sim.append(hdlrun) if self.run_sim: blah blah then - and this should be obvious now - wherever there is like this: if self.run_sim: simrun.setup_during_test() # TODO, some arguments? if self.run_hdl: yield from hdlrun.setup_during_test() replace it with: for runner in list_of_things_to_sim: runner.blabhblah_functon_with_whatever_arguments now, as you can see, from that example, those don't match up: one's a yielder, the other isn't. sooooo..... back iiin.... StateRunner.... we need to do the same trick as before. class StateRunner: def whatever_function_name(self, .....): if False: yield and basically "normalise" the entire API
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=46b475da7b0984d50ad06a2635e223633cbc96c4 i moved pc_i and svstate_i into HDLRunner.
commit 83a798208cee2ebe8a2be6850ab42f75dac5e9d9 (HEAD -> master, origin/master) Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Sat Sep 25 18:49:13 2021 +0100 convert all SimRunner functions to yield - def prepare_for_test(self, test): pass - def run_test(self): pass - def end_test(self): pass - def cleanup(self): pass + def setup_for_test(self): + if False: yield + def setup_during_test(self): + if False: yield
commit ffeac1d44f17114d4363099fd91bef3e180e7406 Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net> Date: Sat Sep 25 18:51:53 2021 +0100 use yield from on StateRunners if self.run_sim: - simrun.setup_during_test() # TODO, some arguments? + yield from simrun.setup_during_test() # TODO, some arguments? etc. ok so that should now be dead easy to convert to the listing format in comment #73, at which point i think it is ready to cut out the entirety of TestRunner over to openpower-isa let me think... what's stopping that from happening... oh: another of those "Factories". i'll sort that. [done https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=82792c9dacf0324ba96f16e112b2a315d0266eca ]
https://git.libre-soc.org/?p=soc.git;a=commit;h=775bdd10f61d75c9130a14f4e4b4ec36c47af7f9 calls except for the one moved over to list format
(In reply to klehman9 from comment #77) > https://git.libre-soc.org/?p=soc.git;a=commit; > h=775bdd10f61d75c9130a14f4e4b4ec36c47af7f9 > > calls except for the one moved over to list format yeah saw it, looks great. i'll move TestRunner into openpower-isa and i think we're done with this one.
Something simple for self.rom https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=c6beaa06b12bd67fcd28999ef7cfe716195f351f As a test to verify code path I tried a 0:0 as the rom with microwatt set to true. Asserts with a r3 unequal. Any other memory location doesn't.
(In reply to klehman9 from comment #79) > Something simple for self.rom > > https://git.libre-soc.org/?p=openpower-isa.git;a=commit; > h=c6beaa06b12bd67fcd28999ef7cfe716195f351f > > As a test to verify code path I tried a 0:0 as the rom with microwatt set to > true. Asserts with a r3 unequal. Any other memory location doesn't. okaay, awesome. that was the perfect temporary hack. good enough for this round, let's think of something else to do. qemu if you like.