Bug 972 - addme/subfme carry/overflow is incorrect
Summary: addme/subfme carry/overflow is incorrect
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: Other Linux
: --- major
Assignee: Jacob Lifshay
URL:
: 1041 (view as bug list)
Depends on:
Blocks: 952
  Show dependency treegraph
 
Reported: 2022-10-28 23:19 BST by Jacob Lifshay
Modified: 2024-03-10 07:02 GMT (History)
3 users (show)

See Also:
NLnet milestone: NLnet.2022-08-107.ongoing
total budget (EUR) for completion of task and all subtasks: 1000
budget (EUR) for this task, excluding subtasks' budget: 1000
parent task for budget allocation: 1027
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
[jacob] amount = 1000 submitted = 2024-02-26 paid = 2024-03-08


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Lifshay 2022-10-28 23:19:48 BST
I added a failing test demonstrating that:
https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=a5e874ed4f20a8febed2f7cf51715e2a930f1f01

SelectableInt should probably propagate carries across multiple add ops
Comment 1 Jacob Lifshay 2022-10-28 23:28:42 BST
imho lots more of our ops likely don't calculate CA correctly because SelectableInt doesn't propagate carries
Comment 2 Luke Kenneth Casson Leighton 2022-10-29 12:15:43 BST
(In reply to Jacob Lifshay from comment #1)
> imho lots more of our ops likely don't calculate CA correctly because
> SelectableInt doesn't propagate carries

see constructor, "overflow".

detecting CA rather unfortunately has to be done manually. this imposition
is expected based on how the Power ISA specification is written.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/caller.py;h=de6719adf90dfb42f59d55539be837631c7906e6;hb=a5e874ed4f20a8febed2f7cf51715e2a930f1f01#l1390

yes, it's awful.

the value "-1" is being inserted into the inputs multiple times
(once by handle_carry() and twice by handle_overflow()) which
obviously it should not be, but it doesn't matter because all
code only looks at the first two inputs (sigh).

i suspect different logic is needed for handling add, analysing
the inputs and outputs in a similar fashion to how CA32 is
done.  a first attempt completely broke all other unit tests,
i'm not up to dealing with this.
Comment 3 Jacob Lifshay 2023-03-26 03:31:58 BST
*** Bug 1041 has been marked as a duplicate of this bug. ***
Comment 4 Jacob Lifshay 2023-03-28 08:35:28 BST
I verified that the test case is in fact correct by checking against POWER9, our simulator is wrong:

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=7f0445f0511984dc2d9c33039919729276d14229
Comment 5 Jacob Lifshay 2023-03-29 04:44:11 BST
I added a bunch of other test cases verified against POWER9, it found 924 failures which afaict are all cases where our simulator gets the wrong ca[32]/ov[32]/so outputs for add-like ops.

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

Now that we have much more complete test coverage, I can attempt to fix the simulator, confident that any errors probably will get caught.
Comment 6 Jacob Lifshay 2023-03-29 04:53:52 BST
full list of erroring instructions (only includes ones I tested):
adde 3, 4, 5
addeo 3, 4, 5
addex 3, 4, 5, 0
addme 3, 4
addmeo 3, 4
addzeo 3, 4
nego 3, 4
subfc 3, 4, 5
subfco 3, 4, 5
subfe 3, 4, 5
subfeo 3, 4, 5
subfic 3, 4, -1
subfme 3, 4
subfmeo 3, 4
subfzeo 3, 4
Comment 7 Jacob Lifshay 2023-03-30 04:06:13 BST
found a weird bug, for `nego.`, get_cr_out is falling through to the `return None, False`. This causes a TypeError when handle_comparison then tries to use that None to index self.crl:

Traceback (most recent call last):
  File "/home/jacob/projects/libreriscv/openpower-isa/src/openpower/test/runner.py", line 306, in process
    insncode)
  File "/home/jacob/projects/libreriscv/openpower-isa/src/openpower/test/runner.py", line 102, in run_test
    yield from sim.execute_one()
  File "/home/jacob/projects/libreriscv/openpower-isa/src/openpower/decoder/isa/caller.py", line 1564, in execute_one
    yield from self.call(opname)         # execute the instruction
  File "/home/jacob/projects/libreriscv/openpower-isa/src/openpower/decoder/isa/caller.py", line 2005, in call
    yield from self.do_rc_ov(ins_name, results[0], overflow, cr0)
  File "/home/jacob/projects/libreriscv/openpower-isa/src/openpower/decoder/isa/caller.py", line 2069, in do_rc_ov
    self.handle_comparison(result, regnum, overflow, no_so=is_setvl)
  File "/home/jacob/projects/libreriscv/openpower-isa/src/openpower/decoder/isa/caller.py", line 1486, in handle_comparison
    self.crl[cr_idx].eq(cr_field)
TypeError: list indices must be integers or slices, not NoneType

pushed a test case:
https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=cd33210ab1516db05cbbf178db02fe889b6cb4f5
Comment 8 Jacob Lifshay 2023-03-30 05:39:57 BST
(In reply to Jacob Lifshay from comment #7)
> found a weird bug, for `nego.`, get_cr_out is falling through to the `return
> None, False`. This causes a TypeError when handle_comparison then tries to
> use that None to index self.crl:

fixed, it, turns out I missed where the csv said NONE where it should have been CR0:

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=f23c637ead369566feb29c1d94da8af14d300512
Comment 9 Jacob Lifshay 2023-03-30 09:04:20 BST
I have the kludge version working for all add-like ops, I also finished adding addex to the simulator as part of fixing CA/OV outputs.

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=7b9a2e4eb5d890a067de2f66eaba81f225fd0e1c;hp=17f19a80183d260238e2cf4ba9b78ccac9fe5807

commit 7b9a2e4eb5d890a067de2f66eaba81f225fd0e1c
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Mar 30 00:55:20 2023 -0700

    fix add-like CA/OV outputs
    
    this is a massive kludge, but that's what lkcl requested due to time constraints

commit f1a4430f6b54872f673375c42bfd301a089df05f
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Mar 30 00:54:22 2023 -0700

    fix broken test case
    
    forgot to set the expected value to the input value for inputs that aren't outputs

commit 158e9113030b59b5391ea996ce7ad906c95ea2fd
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Mar 30 00:02:56 2023 -0700

    add addex to simulator
    
    works, except it has incorrect CA/OV outputs, which I'll fix as part of fixing all add-like ops

commit 168bbcffd49637a20f4f980677de31c83cfdf048
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Mar 29 22:00:43 2023 -0700

    fix typo when getting pseudo-code output variables

commit 06c05d4c7cbd27d40a5846d0fb3f91a99e1c5c93
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Mar 29 21:38:02 2023 -0700

    switch to testing Rc=1 variants

commit f23c637ead369566feb29c1d94da8af14d300512
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Mar 29 21:30:53 2023 -0700

    fix `neg[o].` causing the simulator to raise TypeError

commit cd33210ab1516db05cbbf178db02fe889b6cb4f5
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Mar 29 20:03:43 2023 -0700

    add case_nego_

commit 60410f30b66aa056f74bfea5c710ad81e618bf6b
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Mar 29 19:02:19 2023 -0700

    rename le -> lt since CR bits are lt, gt, eq, and so, not le
Comment 10 Luke Kenneth Casson Leighton 2023-04-04 14:07:11 BST
(In reply to Jacob Lifshay from comment #9)
> I have the kludge version working for all add-like ops, I also finished
> adding addex to the simulator as part of fixing CA/OV outputs.

ahh excellent.
 
>     fix add-like CA/OV outputs
>     
>     this is a massive kludge, but that's what lkcl requested due to time
> constraints

not quite, but given that it works it'll do

commit 6b0a577c9b2de7a6e4c61dab48f8ce01ea0090a1 (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Tue Apr 4 14:02:13 2023 +0100

    comment about massive unnecessary code-duplication that should not
    have been done in the way it was, but it is a good step along the right
    lines because it a gets the job done by b producing the right answers
    that c get us to the simplified path in an incremental fashion.
    am adding this note in the source code to make sure that readers are aware