Bug 686 - create Power ISA test API
Summary: create Power ISA test API
Status: CONFIRMED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Specification (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- enhancement
Assignee: Luke Kenneth Casson Leighton
URL:
: 370 (view as bug list)
Depends on:
Blocks: 242 710 717 738
  Show dependency treegraph
 
Reported: 2021-09-07 13:49 BST by klehman9
Modified: 2021-12-09 14:01 GMT (History)
3 users (show)

See Also:
NLnet milestone: NLNet.2019.10.Standards
total budget (EUR) for completion of task and all subtasks: 1600
budget (EUR) for this task, excluding subtasks' budget: 1600
parent task for budget allocation: 242
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
klehman = 800 [lkcl] amount = 800 submitted = 2021-12-09


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Luke Kenneth Casson Leighton 2021-09-07 14:13:15 BST
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.
Comment 2 Luke Kenneth Casson Leighton 2021-09-07 14:26:41 BST
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.
Comment 3 Luke Kenneth Casson Leighton 2021-09-07 14:48:56 BST
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.
Comment 4 Luke Kenneth Casson Leighton 2021-09-07 14:58:15 BST
*** Bug 370 has been marked as a duplicate of this bug. ***
Comment 5 klehman9 2021-09-07 22:17:32 BST
Start of breaking out functionality of check_regs

https://git.libre-soc.org/?p=soc.git;a=commit;h=dff1ed6ba3b97ab23d67107d35574163f079793c
Comment 6 Luke Kenneth Casson Leighton 2021-09-07 22:43:05 BST
(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.
Comment 7 Luke Kenneth Casson Leighton 2021-09-07 22:43:59 BST
also, each class should not RETURN intregs/simregs, it should STORE
intregs/simregs.  this helps us get around the "yield" problem.
Comment 8 klehman9 2021-09-07 23:36:32 BST
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?
Comment 9 Luke Kenneth Casson Leighton 2021-09-08 01:09:10 BST
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.
Comment 10 klehman9 2021-09-08 14:08:13 BST
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=5eb5477aef6abae55dde17cc5d5f182b32bda10b

modified test_core (not pushed) svp64 passes with class
Comment 11 Luke Kenneth Casson Leighton 2021-09-08 14:21:48 BST
(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".
Comment 12 Luke Kenneth Casson Leighton 2021-09-09 12:06:32 BST
(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.
Comment 13 klehman9 2021-09-09 13:13:31 BST
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.
Comment 14 klehman9 2021-09-09 14:03:54 BST
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()
Comment 15 Luke Kenneth Casson Leighton 2021-09-09 14:12:57 BST
(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"
Comment 16 klehman9 2021-09-09 17:35:24 BST
https://git.libre-soc.org/?p=soc.git;a=commit;h=18aaa0d2de6945f93ff5bb80d3b64e8dce523e6c

hdl portion finished and compared results in test_core
Comment 17 Luke Kenneth Casson Leighton 2021-09-09 18:53:57 BST
(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(...)
Comment 18 Jacob Lifshay 2021-09-09 20:53:12 BST
(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 `[]`
Comment 19 Jacob Lifshay 2021-09-09 21:16:22 BST
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.
Comment 20 klehman9 2021-09-09 22:55:26 BST
https://git.libre-soc.org/?p=soc.git;a=commit;h=b5805e3280ed6a5ad4c86dcfb1b92f58a49cc0bf

Finished generators/general cleanup
Comment 21 Luke Kenneth Casson Leighton 2021-09-09 23:38:12 BST
(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".
Comment 22 Luke Kenneth Casson Leighton 2021-09-09 23:41:16 BST
(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.
Comment 23 Jacob Lifshay 2021-09-09 23:54:26 BST
(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).
Comment 24 Luke Kenneth Casson Leighton 2021-09-10 01:05:49 BST
(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.
Comment 25 klehman9 2021-09-10 16:16:32 BST
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.
Comment 26 Luke Kenneth Casson Leighton 2021-09-10 16:29:43 BST
(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.
Comment 27 Luke Kenneth Casson Leighton 2021-09-10 16:42:57 BST
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.
Comment 28 klehman9 2021-09-10 18:31:24 BST
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.
Comment 29 Luke Kenneth Casson Leighton 2021-09-10 19:10:22 BST
(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
Comment 30 klehman9 2021-09-10 22:02:36 BST
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.
Comment 31 Luke Kenneth Casson Leighton 2021-09-10 22:14:50 BST
(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?
Comment 32 klehman9 2021-09-10 23:04:49 BST
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.
Comment 33 Luke Kenneth Casson Leighton 2021-09-10 23:16:12 BST
(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")
Comment 34 klehman9 2021-09-12 01:59:58 BST
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)
Comment 35 klehman9 2021-09-12 05:00:05 BST
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.
Comment 36 Luke Kenneth Casson Leighton 2021-09-12 10:15:59 BST
(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.
Comment 38 Luke Kenneth Casson Leighton 2021-09-12 14:24:40 BST
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.
Comment 39 klehman9 2021-09-14 16:47:52 BST
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.
Comment 40 Luke Kenneth Casson Leighton 2021-09-14 18:47:24 BST
(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
Comment 41 Jacob Lifshay 2021-09-14 19:13:50 BST
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.
Comment 42 Luke Kenneth Casson Leighton 2021-09-14 19:31:23 BST
(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.
Comment 43 Jacob Lifshay 2021-09-14 19:49:13 BST
(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)))
Comment 44 Luke Kenneth Casson Leighton 2021-09-14 20:08:54 BST
(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.
Comment 46 Luke Kenneth Casson Leighton 2021-09-15 21:12:21 BST
(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.
Comment 47 klehman9 2021-09-16 15:58:33 BST
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.
Comment 48 Luke Kenneth Casson Leighton 2021-09-16 16:34:20 BST
(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.
Comment 49 Luke Kenneth Casson Leighton 2021-09-16 17:08:37 BST
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
Comment 51 Luke Kenneth Casson Leighton 2021-09-17 16:12:35 BST
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
Comment 52 Luke Kenneth Casson Leighton 2021-09-17 19:35:29 BST
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
Comment 53 klehman9 2021-09-18 12:53:02 BST
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.
Comment 54 Luke Kenneth Casson Leighton 2021-09-18 13:10:18 BST
(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.
Comment 56 Luke Kenneth Casson Leighton 2021-09-21 20:45:40 BST
(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.
Comment 57 Luke Kenneth Casson Leighton 2021-09-22 05:20:54 BST
https://www.programiz.com/python-programming/methods/set/difference

found it. "difference"
Comment 58 klehman9 2021-09-22 14:55:54 BST
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.
Comment 59 Luke Kenneth Casson Leighton 2021-09-22 15:20:44 BST
(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.
Comment 60 Luke Kenneth Casson Leighton 2021-09-22 17:17:59 BST
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
Comment 61 Luke Kenneth Casson Leighton 2021-09-22 19:49:15 BST
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.
Comment 62 Luke Kenneth Casson Leighton 2021-09-22 21:17:00 BST
(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
Comment 63 Luke Kenneth Casson Leighton 2021-09-22 21:52:48 BST
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
Comment 65 Luke Kenneth Casson Leighton 2021-09-22 23:08:46 BST
(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.
Comment 66 klehman9 2021-09-23 00:08:20 BST
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.
Comment 67 Luke Kenneth Casson Leighton 2021-09-23 23:40:27 BST
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
Comment 68 Luke Kenneth Casson Leighton 2021-09-23 23:55:07 BST
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)
Comment 69 klehman9 2021-09-25 13:19:37 BST
https://git.libre-soc.org/?p=soc.git;a=commit;h=c5213803b82fd4d348c90fefff2a62eeb4c814d2

hdlrunner class with test_runner changes to use it
Comment 70 Luke Kenneth Casson Leighton 2021-09-25 13:29:34 BST
(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?
Comment 71 Luke Kenneth Casson Leighton 2021-09-25 13:36:51 BST
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=5bc2cf02f0df7639fbc6b2869b1d9cba71b9c8d2

moved the call to sim test-running behind the (evolving) SimRunner API
Comment 73 Luke Kenneth Casson Leighton 2021-09-25 16:30:52 BST
(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
Comment 74 Luke Kenneth Casson Leighton 2021-09-25 16:31:23 BST
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=46b475da7b0984d50ad06a2635e223633cbc96c4

i moved pc_i and svstate_i into HDLRunner.
Comment 75 Luke Kenneth Casson Leighton 2021-09-25 18:50:00 BST
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
Comment 76 Luke Kenneth Casson Leighton 2021-09-25 19:00:18 BST
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 ]
Comment 77 klehman9 2021-09-26 12:50:16 BST
https://git.libre-soc.org/?p=soc.git;a=commit;h=775bdd10f61d75c9130a14f4e4b4ec36c47af7f9

calls except for the one moved over to list format
Comment 78 Luke Kenneth Casson Leighton 2021-09-30 07:38:25 BST
(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.
Comment 79 klehman9 2021-10-01 23:14:19 BST
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.
Comment 80 Luke Kenneth Casson Leighton 2021-10-01 23:44:07 BST
(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.