additional context:
https://libre-soc.org/irclog/%23libre-soc.2021-10-20.log.html#t2021-10-20T14:14:55

if you cut/paste and adapt this to use alu_test_cases.py
that will give you a "way to run the test cases"

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/test_caller_shift_rot.py;hb=HEAD

kyle i leave it with you to work together on this, help each other out?

Veera, start small.

Get the new test caller and new cases file running as is... then commit them.  Then we can start on adding expected results.  This step should only take a few changes based on Luke's links above.

Need help for understanding the construct and meaning.

def case_addis_nonzero_r0_regression(self):

lst = [f"addis 3, 0, 1"]

Here 3 means GPR3 register, 0 means GPR0 register and value of SI=1

initial_regs[0] = 5

here GPR0 is assigned value 5

e = ExpectedState(initial_regs, pc=4) ||| What does it do?

e.intregs[3] = 0x10000
is it here GPR3 is reassigned the value of 0x10000

self.add_case(Program(lst, bigendian), initial_regs, expected=e) ||| what this does?

(In reply to klehman9 from comment #2)
> Veera, start small.
> 
> Get the new test caller and new cases file running as is... then commit
> them.  Then we can start on adding expected results.  This step should only
> take a few changes based on Luke's links above.

Added test_caller_alu.py

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/test_caller_alu.py;h=155d1a98011cb7b02aa28de11dafc353ab42a921;hb=HEAD

(In reply to vklr@vkten.in from comment #3)
> Need help for understanding the construct and meaning.
> 
> def case_addis_nonzero_r0_regression(self):
> lst = [f"addis 3, 0, 1"]
> Here 3 means GPR3 register, 0 means GPR0 register and value of SI=1

correct.

> initial_regs[0] = 5
> here GPR0 is assigned value 5

so on startup of the simulator, the initial reg 0 will be 5, yes.
 
> e = ExpectedState(initial_regs, pc=4) ||| What does it do?

this says "we expect the state to be what is in the variable e" 

> e.intregs[3] = 0x10000
> is it here GPR3 is reassigned the value of 0x10000

and ExpectedState took a *copy* of its input GPR table,
which was "e.intregs[0] = 5" already (GPR0=5), now GPR3 is modified
to 0x10000

so, on completion of the execution of "addis GPR3 GPR0 1",
we EXPECT that GPR0 is still 5,
we EXPECT that GPR3 is 0x10000,
we EXPECT that pc=4.



> self.add_case(Program(lst, bigendian), initial_regs, expected=e) ||| what
> this does?

it says, "please add a case (add_case) of a Program to be run
(here is the assembly listing, lst), with the initial regs,
and when completed we EXPECT the results to be as we defined
in the variable e"

it is like reading English, but with some words missing.

(In reply to Luke Kenneth Casson Leighton from comment #5)
> (In reply to vklr@vkten.in from comment #3)
> > Need help for understanding the construct and meaning.
> > 
> > initial_regs[0] = 5
> > here GPR0 is assigned value 5
> 
> so on startup of the simulator, the initial reg 0 will be 5, yes.
>  
> > e.intregs[3] = 0x10000
> > is it here GPR3 is reassigned the value of 0x10000
> 
> and ExpectedState took a *copy* of its input GPR table,
> which was "e.intregs[0] = 5" already (GPR0=5), now GPR3 is modified
> to 0x10000
> 

Shouldn't GPR3 be 5 + 0x10000 = 0x10005

> so, on completion of the execution of "addis GPR3 GPR0 1",
> we EXPECT that GPR0 is still 5,
> we EXPECT that GPR3 is 0x10000,
> we EXPECT that pc=4.
>

You would think, and call it a quirk, R0 is usually handled differently.  That's why in a lot of tests you see registers 3,2,1.

If you look at the psuedo code in PowerISA manual, you'll see
  if RA = 0 then RT <-EXTS(SI || 16 0)
    else RT <- (RA) + EXTS(SI || 16 0)

So basically when RA is 0 its value is ignored and the result is only shifted.

(In reply to klehman9 from comment #7)
> You would think, and call it a quirk, R0 is usually handled differently. 
> That's why in a lot of tests you see registers 3,2,1.
> 
> If you look at the psuedo code in PowerISA manual, you'll see
>   if RA = 0 then RT <-EXTS(SI || 16 0)
>     else RT <- (RA) + EXTS(SI || 16 0)
> 
> So basically when RA is 0 its value is ignored and the result is only
> shifted.

In here RA = GPR0 = 5

RT <- 5 + EXTS(1 || 16 0)

The instruction addis 3, 0, 1

In this case RA is GPR0. Not the value of GPR0.

So the instruction is asking if RA is GPR0?  It is so shift.

If RA wasn't GPR0, it takes the value in there and adds it.

So if you did:
addis 3,4,1
initial_regs[4] = 5

then you would get 0x10005

(In reply to klehman9 from comment #7)
> You would think, and call it a quirk, R0 is usually handled differently. 
> That's why in a lot of tests you see registers 3,2,1.
> 
> If you look at the psuedo code in PowerISA manual, you'll see
>   if RA = 0 then RT <-EXTS(SI || 16 0)
>     else RT <- (RA) + EXTS(SI || 16 0)
> 
> So basically when RA is 0 its value is ignored and the result is only
> shifted.

from section 1.3.2 page 4 of Power ISA 3.0 Book I:

 (RA|0) means the contents of register RA if the RA
 field has the value 1-31, or the value 0 if the RA
 field is 0.

so, from the pseudocode at this line:

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=openpower/isa/fixedarith.mdwn;h=46f635294a0ecda189782a11ae985d443a674578;hb=4495f61303afca9790a9e2c3bab4cf8977c7de11#l27

  19 # Add Immediate Shifted
  20 
  21 D-Form
  22 
  23 * addis RT,RA,SI
  24 
  25 Pseudo-code:
  26 
  27     RT <- (RA|0) + EXTS(SI || [0]*16)

that "(RA|0)" is equivalent to what kyle wrote.

sometimes, bizarrely, you see this:

   if RA = 0 then
     b <- 0 
   else
     b <- (RA)

which i have no idea why the pseudocode writers did not just use

    b <- (RA|0)

perhaps one day we will be able to contact them and find out

kyle, i just realised, there is a neat trick that
can be used to massively accelerate this process.

could you add a function to ExpectedState which dumps
its contents into a /tmp/testname_subtest_name.py that
looks exactly like this:

      e = ExpectedState(pc=xxx)
      e.intregs[yy] = 0xZZ

etc etc.

anything that is zero just exclude it.

then make the test API call that dump function at the
end.

it then becomes a near trivial matter to cut/paste those
codefragments into every unit test.

https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=a574480267c1ac561c171a3885777beef567ce1e

Dumps last state to tmp/test_case_name.py for creating and setting expected
state.

If expected state passed into runner, no file is created.

The test case name was the only name I could get to create the file name as far as I can tell.

https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=5ada14e4fed3098edac3449cc880b975c68733fb

adding a parameter to passthrough a name of a class of test cases (or whatever) via add_case in common.py would be trivial (well besides adding it to all the cases) for directory sorting.

If that approach is ok, then standardize on self.__class__ to pass?

(In reply to klehman9 from comment #13)
> https://git.libre-soc.org/?p=openpower-isa.git;a=commit;
> h=5ada14e4fed3098edac3449cc880b975c68733fb

looks great.

> adding a parameter to passthrough a name of a class of test cases (or
> whatever) via add_case in common.py would be trivial (well besides adding it
> to all the cases) for directory sorting.

the problem with adding it as an argument is that it is
added to literally everything.  hundreds of unit tests.

a more standard approach, one adopted by python unittest, is to inspect
the stack frame and obtain the name of the caller *of* the add_cases
function.

i believe this has already been done.

the next upper level of hierarchy is to separate test classes, by
determining the class name: self.__class__.__name__ does the trick.
use that in add_cases() to add an extra piece of info to the
test_data, it can then propagate through and eventually get put
as an argument to the dump function

quite laborious but hey

https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=1b2f031b2a2cc3b6473f4965345f21547164c5c3

https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=0e32ffeccef54c27f8d7300260fd75eecb4dde2e

Sub directories now created based on the test cases filename.

(In reply to klehman9 from comment #15)

> Sub directories now created based on the test cases filename.

that'll do perfectly.

ok, so, veera, now you have, when you run the test, a file
created in /tmp which contains the code that you can put
actually into each test case.

you do not have to laboriously inspect the debug log output
looking for the information.

start with just one case_xxx

(In reply to klehman9 from comment #15)
> https://git.libre-soc.org/?p=openpower-isa.git;a=commit;
> h=1b2f031b2a2cc3b6473f4965345f21547164c5c3
> 
> https://git.libre-soc.org/?p=openpower-isa.git;a=commit;
> h=0e32ffeccef54c27f8d7300260fd75eecb4dde2e
> 
> Sub directories now created based on the test cases filename.

You can use get_test_path from nmutil, it handles creating a unique path each time it's called that is derived from the test case's name as well as the current test method's name:

https://git.libre-soc.org/?p=nmutil.git;a=blob;f=src/nmutil/get_test_path.py;h=52bd53066d2254f87888a9c2b549ee3d5776b36d;hb=HEAD

example code (in src/demo.py):

class MyTest(unittest.TestCase):
    def test_fn(self):
        # code below should be put in common function:
        path = get_test_path(test_case, "test_outputs")
        # path is a pathlib.Path instance
        path.mkdir(parents=True, exist_ok=True)
        file_path = path / "file.py"
        with open(file_path, "wt") as file:
            pass # write to file

that creates a file:
test_outputs/src.demo.MyTest.test_fn/0/file.py

where the 0 is automatically incremented if you call get_test_path more than once in the same unit test function.

/test_outputs should be added to .gitignore

I did a very similar thing in:
https://git.libre-soc.org/?p=ieee754fpu.git;a=blob;f=src/ieee754/partitioned_signal_tester.py;h=2986ee440e334cf2dd700db050fd76bbca323674;hb=HEAD#l352

https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=0b7eb1cc2b6f1b820a54e668724f1e00967e85f3

cr regs were reversed

def case_cmp(self):
 151         lst = ["subf. 1, 6, 7",
 152                "cmp cr2, 1, 6, 7"]
 153         initial_regs = [0] * 32
 154         initial_regs[6] = 0x10
 155         initial_regs[7] = 0x05

running test_caller_alu.py shows:
        e = ExpectedState(pc=8)
        e.intregs[1] = 0xfffffffffffffff5
        e.intregs[6] = 0x10
        e.intregs[7] = 0x5
        e.crregs[0] = 0x8
        e.crregs[2] = 0x4

for subf. 1, 6, 7
here gpr1 is 0xffff_ffff_ffff_fff5 lesser than 0 so cr0 BF 1000

for cmp cr2, 1, 6, 7
here gpr6 is 0x10 bigger than gpr7 0x5 so cr2 should be 0b010 instead of now evaluated 0b100

(In reply to vklr@vkten.in from comment #19)
> def case_cmp(self):

> for cmp cr2, 1, 6, 7
> here gpr6 is 0x10 bigger than gpr7 0x5 so cr2 should be 0b010 instead of now
> evaluated 0b100

veera: we know that the instructions produce the right answer, they have
been tested against microwatt, etc etc.

therefore, we conclude that your current understanding of cmp
must be wrong.  i suspect you may have missed that for cmp, for
some bizarre reason, they swapped the args as compared to subf.

so to get the results *you* are expecting, it would have to be
cmp cr2, 1, 7, 6 not cmp cr2, 1, 6, 7 

basically this is very much a by-rote task (i apologise), except
when it comes to tests that have random input, in which case you
will need to actually compute the expected results "by hand" (python)

trust that the output is correct, ok? it's good you're reviewing
it though.

(In reply to klehman9 from comment #18)
> https://git.libre-soc.org/?p=openpower-isa.git;a=commit;
> h=0b7eb1cc2b6f1b820a54e668724f1e00967e85f3
> 
> cr regs were reversed

sigh, yes, Because Power ISA.  MSB0 numbering (VHDL "upto" not
"downto")

you would not believe how long it took to get things right and
get rid of bugs related to CR bit-ordering (MSB0). five frickin
months.

Test fails for:
def case_cmp(self):
 lst = ["subf. 1, 6, 7",
        "cmp cr2, 1, 6, 7"]
        initial_regs = [0] * 32
        initial_regs[6] = 0x10
        initial_regs[7] = 0x05
        e = ExpectedState(pc=8)
        e.intregs[1] = 0xfffffffffffffff5
        e.intregs[6] = 0x10
        e.intregs[7] = 0x5
        e.crregs[0] = 0x8
        e.crregs[2] = 0x4
        self.add_case(Program(lst, bigendian), initial_regs, expected=e)

I have put in the expected things as generated in /tmp folder.

When I rerun get this failure notice:

asserting...cr 0 0 8

======================================================================
FAIL: run_all (soc.simple.test.test_runner.TestRunner) [case_cmp]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vklr/src/openpower-isa/src/openpower/test/runner.py", line 273, in process
    last_sim.compare(test.expected)
  File "/home/vklr/src/openpower-isa/src/openpower/test/state.py", line 97, in compare
    self.crregs, s2.crregs))
AssertionError: 0 != 8 : cr reg 0 (sim) not equal (expected) 'cmp cr2, 1, 6, 7'. got 0  expected 8

----------------------------------------------------------------------
Ran 1 test in 3.053s

FAILED (failures=1)

---
What can be done?

The test output shows register values in decimal. Can it be made to output in hex.

asserting...reg 0 0 0
code, frepr(code) cmp cr2, 1, 6, 7 'cmp cr2, 1, 6, 7'
asserting...reg 1 18446744073709551605 18446744073709551605

18446744073709551605 = 0xfffffffffffffff5

(In reply to vklr@vkten.in from comment #21)
> ---
> What can be done?

this means very simply that the code-generator is outputting
the cr values in the wrong bit-order.

whilst kyle is looking at that, you should always assume
that the simulator is 100% correct, at all times.

therefore, automatically and without needing to ask every
time an assertion is encountered, you should correct the
expected results to match.

this should be an automatic and immediate reaction because,
as i explained in previous comments, we know that the simulator
and HDL match.

         e.crregs[0] = 0x1
         e.crregs[2] = 0x2

kyle, there is a function in SelectableInt which
allows extraction of the value in LSB0 order.
you do not have to write one manually which inverts
the bitorder.

kyle:

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/selectable_int.py;h=34ea01550174df96a7cd13896d0775e33575c7b8;hb=a17a252e474d5d5bf34026c25a19682e3f2015c3#l125

if any register is a SelectableInt you will need to call
asint(lsb0=True)

Add expected state to case_1_regression for extsw for alu_cases unit test

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=5e8f8402f6d6ec8a251803d619e10106cb8359b3

I hope code format and placement is ok.
Kindly review and give feedback.

(In reply to vklr@vkten.in from comment #25)
> Add expected state to case_1_regression for extsw for alu_cases unit test
> 
> https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;
> h=5e8f8402f6d6ec8a251803d619e10106cb8359b3
> 
> I hope code format and placement is ok.
> Kindly review and give feedback.

looks great, real easy, isn't it? if you can avoid the difficult ones
for now, while getting the hang of it.

also avoid instructions with CRs until we can investigate.  kyle, line 215,
probably the dump function might need to output a SelectableInt, or
something?

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/test_caller_svp64.py;h=0265b2ae9caadc713695430441ba39b5f1c9fbed;hb=5e8f8402f6d6ec8a251803d619e10106cb8359b3#l215

Yeah, solution I'm thinking of for this is to revert my last fix which basically sets the cr bits properly in expected.  It just copied from a last state which, while bit order is correct, is reversed from the readable assembly instruction where the confusion started.

So in state, register CR7 would show up in the expect code as expected = cr[0].  CR6 is cr[1] etc.

I can add a comment on the end of those lines making the human reader/tester more aware of how they are handled such as # Set CR7 even though it looks like assigning something to expected.cr[0].

(In reply to klehman9 from comment #27)

> So in state, register CR7 would show up in the expect code as expected =
> cr[0].  CR6 is cr[1] etc.

ah... no.  it's more complicated than that: the individual 4-bit CR Fields
are in MSB0 bit order (!)

the numbering we chose CR0-CR7 meets cr[0] and cr[7] etc.

> I can add a comment on the end of those lines making the human reader/tester
> more aware of how they are handled such as # Set CR7 even though it looks
> like assigning something to expected.cr[0].

nope :)

however... when those 8 4-bit CR Fields are placed into the 32-bit CR register,
*THAT* is where you must do the inversion of the numbering!

what will make you totally cry and go bananas is that IBM actually made
the CR register 64-bit

and, because of the MSB0 numbering, the 8 4-bit CR fields do not go into
CR_register[0..31], they go into CR_register[32..63]!!!

as if that wasn't enough, python end-point is inclusive so it's actually
CR_register[32:64]

at this point your brain should have melted or you should be screaming
and/or bashing your head against the nearest spiked metal object in order
to stop the pain, exasperation, and abject horror.

Ha, horror is a way to describe it.

Not discounting what you wrote at all about bit ordering.  I think we are on the same page but different paragraphs ;-).

All I'm/it is doing is taking a state object, and creating an expected state from it.  Those registers are pulled and stored in whatever fashion before that part hits.  What's in the registers, how it is stored doesn't matter for this simple compare process.  Those expected cr assignment statements may look weird but the test will pass through test issuer with flying colors since it will be comparing an expected state verbatim with whatever last_state was bit ordering be darned.

Now whether or not that needs to be changed (which now that I have written all this out...it might have dawned on me that is what you are shooting for), I'm all ears for that one and that of course would drag the bits into the conversation.

hilarious :)  i think what will happen is, as Veera progresses and
encounters more with CR Fields, we we can look at them then.
i'd like to see more examples basically.

Add expected state to case_1_regression for subf in alu_cases unit test

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=f3fe1effe787713add876d1a60a37c387bfafa5b

Kindly review and give feedback.

(In reply to vklr@vkten.in from comment #31)
> Add expected state to case_1_regression for subf in alu_cases unit test
> 
> https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;
> h=f3fe1effe787713add876d1a60a37c387bfafa5b
> 
> Kindly review and give feedback.

looks great, keep going: do the easy ones first, if you run into
a problem one, raise it here, otherwise just keep doing:

* look at dump
* find one suitable simple expected state
* add to unit test
* re-run unit test
* check it passes
* git diff (to review)
* git commit
* git push
* repeat

you should be able to accelerate and do 10 of those commits a day,
minimum.  as long as they pass the unit test no need to ask for
review, just do it.

I just git pulled all:
c4m-jtag  ieee754fpu  nmigen  nmigen-soc  nmutil  openpower-isa  soc

and get this error by running:

python3 test_caller_alu.py
Traceback (most recent call last):
  File "test_caller_alu.py", line 13, in <module>
    from soc.simple.test.test_runner import TestRunner
  File "/home/vklr/src/soc/src/soc/simple/test/test_runner.py", line 17, in <module>
    from openpower.decoder.isa.all import ISA
ModuleNotFoundError: No module named 'openpower.decoder.isa.all'

(In reply to vklr@vkten.in from comment #33)
> ModuleNotFoundError: No module named 'openpower.decoder.isa.all'

did you remember to rerun `make pywriter` in openpower?

(In reply to klehman9 from comment #27)
> Yeah, solution I'm thinking of for this is to revert my last fix which
> basically sets the cr bits properly in expected. 

yes, this was the error - the code i wrote already took care of CR field
numbering.

however this error was masked by another error:

     def compare(self, s2):
         # Compare int registers
-        for i, (self.intregs, s2.intregs) in enumerate(
+        for i, (intreg, intreg2) in enumerate(

         # CR registers
-        for i, (self.crregs, s2.crregs) in enumerate(
+        for i, (crreg, crreg2) in enumerate(
                 zip(self.crregs, s2.crregs)):


that was *destroying* self.crregs and s2.crregs, leaving a CR field
value (modifying) both self.crregs and s2.crregs

i've sorted that (investigating with Veera) and it's fine now.

There is probably a typing mistake (creating a bug) in:

def case_cmplw_microwatt_1(self):
   lst = ["cmpl 1, 0, 22, 4"]

Perhaps it should have been:
   lst = ["cmplw 1, 0, 22, 4"]

Please verify and let me know!

(In reply to vklr@vkten.in from comment #36)
> There is probably a typing mistake (creating a bug) in:
> 
> def case_cmplw_microwatt_1(self):
>    lst = ["cmpl 1, 0, 22, 4"]
> 
> Perhaps it should have been:
>    lst = ["cmplw 1, 0, 22, 4"]
> 
> Please verify and let me know!

Just read manual again (I missed previously) which says:

cmplw cr3,Rx,Ry
eqv to
cmpl 3,0,Rx,Ry

I have put the expected state codes to all in which it could be put.

Left ones are which use random values or range choice.

Kindly give feedback about bug completion status.

If any left I will sure try to do it.

(In reply to vklr@vkten.in from comment #37)
 
> Just read manual again (I missed previously) which says:
> 
> cmplw cr3,Rx,Ry
> eqv to
> cmpl 3,0,Rx,Ry

yes. section 3.3.10 p82 book I v3.0C.
aliases (pseudo ops aka "extended menonics")
are not supported by ISACaller and never will be.

to reiterate: you should always take all unit test
assembly code to be 100% correct.

(In reply to vklr@vkten.in from comment #38)
> I have put the expected state codes to all in which it could be put.

brilliant.

> Left ones are which use random values or range choice.

ok, those will be next (the random value ones), or, if
fixed values with range, you can see the style i did
in one example, i analysed the expected results dump and did
on-the-fly construction.

kyle if you'd like to help with that feel free

> Kindly give feedback about bug completion status.
> 
> If any left I will sure try to do it.

Created attachment 146 [details]
alu_test_case case_0_adde_expected

ALU unit test case: case_0_adde_expected

Created attachment 147 [details]
alu_test_case case_0_adde_expected.log

ALU unit test case log: case_0_adde_expected.log

ALU Test unit case case_0_adde_expected

log shows: asserting...cr 0 4 0

it should have been: asserting...cr 0 4 4

See:
https://bugs.libre-soc.org/attachment.cgi?id=146
https://bugs.libre-soc.org/attachment.cgi?id=147

Resulting in result failure!

also I think adde. RT,RA,RB clears CA bit to 0.

(In reply to vklr@vkten.in from comment #42)
> ALU Test unit case case_0_adde_expected
> 
> log shows: asserting...cr 0 4 0
> 
> it should have been: asserting...cr 0 4 4
> 
> See:
> https://bugs.libre-soc.org/attachment.cgi?id=146
> https://bugs.libre-soc.org/attachment.cgi?id=147
> 
> Resulting in result failure!
> 
> also I think adde. RT,RA,RB clears CA bit to 0.

Excuses I failed to set gt to 1 (gt = 1).

Got fixed.

But still have problems with CA value: it is varying with each test.
I will work out.

(In reply to vklr@vkten.in from comment #43)

> But still have problems with CA value: it is varying with each test.
> I will work out.

CA and CA32 are bit 64 (65th bit) and bit 32 (33rd bit).
see comments in diff, link in irclog

i do not remember if XER.ca == CA | CA32<<1
or if it is CA32 | CA<<1

I am unable to work out expected values for e.ca in case_0_adde_expected.

I do not have the capability to do any further work.

Can somebody else do it for random choice/number test cases.

Added expected state to case_extsb

It does not needed CA or CA32 bit.

In case_0_adde_expected I could not permute and combinate the possible outcomes.
I think only those who know the OPENPOWER architecture can find out!

(In reply to vklr@vkten.in from comment #46)
> In case_0_adde_expected I could not permute and combinate the possible
> outcomes.
> I think only those who know the OPENPOWER architecture can find out!

I successfully worked out case_0_adde and added the expected state!

(In reply to vklr@vkten.in from comment #46)
> Added expected state to case_extsb
> 
> It does not needed CA or CA32 bit.

yes that sounds right.  in the PDF spec you see "flags" for
each instruction: there is no "extsbo" - no OE=1 - and
this is because carry and carry32 are meaningless for extsb.
 
> In case_0_adde_expected I could not permute and combinate the possible
> outcomes.
> I think only those who know the OPENPOWER architecture can find out!

case_0_adde_extended is a copy of case_0_adde...

(In reply to vklr@vkten.in from comment #47)

> I successfully worked out case_0_adde and added the expected state!

... which you managed to do here, hooray!

Well I have added expected state codes to all cases and they all pass without fail.

Luke and others, kindly review and verify it and tell me if something wrong.

Luke, may you allot money for this bug(tentative).

test this != 0 just like line 304

 305             carry_out32 = ((initial_regs[6] & 0xffff_ffff) + \
 306                     (initial_regs[7] & 0xffff_ffff)) & (1<<32)

then make this (carry_out32 << 1)

 323             e.ca = carry_out | (carry_out32>>31)

same everywhere else.

 260                 if result < 0:
 261                     e.intregs[3] = (result + 2**64) & ((2**64)-1)
 262                 else:
 263                     e.intregs[3] = result & ((2**64)-1)

can just be:

 263                     e.intregs[3] = result & ((2**64)-1)

(a negative number in python is automatically made a +ve
 result if ANDed.  -5 & 0b1 == 1 for example)

 272                 if imm >= 0:
 273                     value = (~initial_regs[1]+2**64) + (imm) + 1
 274                 else:
 275                     value = (~initial_regs[1]+2**64) + (imm+2**64) + 1

can become:

    value = (~initial_regs[1]+2**64) + (imm) + 1
    if imm < 0:
        value += 2**64

always look for patterns like that, rather than duplicate complex
identical expressions.

(In reply to Luke Kenneth Casson Leighton from comment #52)
> can become:
> 
>     value = (~initial_regs[1]+2**64) + (imm) + 1
>     if imm < 0:
>         value += 2**64
> 
> always look for patterns like that, rather than duplicate complex
> identical expressions.

For this comment and comment #51, made the code short. Checked it by running, it passes.

(In reply to vklr@vkten.in from comment #53)

> For this comment and comment #51, made the code short. Checked it by
> running, it passes.

 270                 if imm < 0:
 271                     value =+ 2**64
                               ^^
should have been:

> >     if imm < 0:
> >         value += 2**64
                  ^^

 240             if imm < 0:
 241                 e.intregs[3] = (imm + 2**48)<<16
 242             else:
 243                 e.intregs[3] = imm << 16

can i believe be just:

                 e.intregs[3] = (imm << 16) & ((1<<64)-1)

although this may need checking.  again, this relies on
-ve numbers becoming +ve when &ed.

-1 & 0b11 ==> 0b11 (i.e. not -0b11)

(In reply to Luke Kenneth Casson Leighton from comment #55)
>  240             if imm < 0:
>  241                 e.intregs[3] = (imm + 2**48)<<16
>  242             else:
>  243                 e.intregs[3] = imm << 16
> 
> can i believe be just:
> 
>                  e.intregs[3] = (imm << 16) & ((1<<64)-1)
> 
> although this may need checking.  again, this relies on
> -ve numbers becoming +ve when &ed.
> 
> -1 & 0b11 ==> 0b11 (i.e. not -0b11)

Corrected the add-equal operator bug as per comment 54.

Yes using bitwise & op makes neg operand +ve, checked it in python3 prompt. So shortened the code as you put here. And it passed the tests.

In reply to chat in IRC

I have put shortened code as much as possible. I think it cannot be shortened any further. Please give a look and give feedback.

Cases:
 case_addis_nonzero_r0
 case_rand_imm
 case_rand
 case_extsb

adde yeah looks great

 506                 s = ((initial_regs[1] & 0x1000_0000_0000_0080)>>7)&0x1
 507                 if s == 1:
 508                     value = 0xffff_ffff_ffff_ff<<8
 509                 else:
 510                     value = 0x0
 511                 e.intregs[3] = value | (initial_regs[1] & 0xff)

this can go into a function... ah! i found one

  36 def exts(value, bits):
  37     sign = 1 << (bits - 1)
  38     return (value & (sign - 1)) - (value & sign)

use this:

from openpower.helpers import exts

then 506 to 511 is replaced with:

506      e.intregs[3] = exts(initialregs[1], 8) & ((1<<64)-1)

same for extsh and extsw except 16 and 32 as arg not 8

(In reply to Luke Kenneth Casson Leighton from comment #58)
> adde yeah looks great
> this can go into a function... ah! i found one
> 
>   36 def exts(value, bits):
>   37     sign = 1 << (bits - 1)
>   38     return (value & (sign - 1)) - (value & sign)
> 
> use this:
> 
> from openpower.helpers import exts

Running tests says: no module named openpower.helpers

git grep can't find it in both openpower-isa and soc repo

(In reply to vklr@vkten.in from comment #59)

> Running tests says: no module named openpower.helpers
> 
> git grep can't find it in both openpower-isa and soc repo

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/helpers.py;h=8caa10689b595ddc77a0f457ccfa9f2875e753a3;hb=f0d44c0fbd77425385aef26b51b88af574102f6d#l36

(In reply to Luke Kenneth Casson Leighton from comment #58)
> adde yeah looks great
> this can go into a function... ah! i found one
> 
>   36 def exts(value, bits):
>
> use this:
> 
> from openpower.helpers import exts
> 
> then 506 to 511 is replaced with:
> 
> 506      e.intregs[3] = exts(initialregs[1], 8) & ((1<<64)-1)
> 
> same for extsh and extsw except 16 and 32 as arg not 8

Done. I have updated the case_extsb with this exts function. And tests pass successfully.

(In reply to vklr@vkten.in from comment #61)

> Done. I have updated the case_extsb with this exts function. And tests pass
> successfully.

brilliant, ok i think that's good. some comments might be useful
but are not strictly necessary, i think this one can be closed.
kyle i added this one to your page so it is not lost