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
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