| Summary: | addme/subfme carry/overflow is incorrect | ||
|---|---|---|---|
| Product: | Libre-SOC's first SoC | Reporter: | Jacob Lifshay <programmerjake> |
| Component: | Source Code | Assignee: | Jacob Lifshay <programmerjake> |
| Status: | RESOLVED FIXED | ||
| Severity: | major | CC: | libre-soc-bugs, lkcl, programmerjake |
| Priority: | --- | ||
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | Linux | ||
| See Also: | https://bugs.libre-soc.org/show_bug.cgi?id=946 | ||
| 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
|
|
| Bug Depends on: | |||
| Bug Blocks: | 952 | ||
|
Description
Jacob Lifshay
2022-10-28 23:19:48 BST
imho lots more of our ops likely don't calculate CA correctly because SelectableInt doesn't propagate carries (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. *** Bug 1041 has been marked as a duplicate of this bug. *** 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 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. 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 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
(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 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 (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 |