https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=4e701a851536bba6648779c183293ba75e7ea7b8 commit 4e701a851536bba6648779c183293ba75e7ea7b8 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Mon Apr 24 23:15:11 2023 -0700 fix bug where pseudo-code assignments modify more than just the variable being assigned to I added `copy_assign_rhs` calls to all pseudo-code assignment RHSes so that SelectableInt instances (and whatever other stuff is needed) can be copied. Example from commit: # this needs to copy the SelectableInt instance in RA # not just assign a reference to it to A A <- RA A[0] <- 1 # if the copy wasn't performed, we just modified RA too! This shows up in the `minmax` pseudo-code: b <- (RB) ... b[0] <- ¬b[0] before this fix, that pseudo-code undesirably modified the value in the GPR!
(In reply to Jacob Lifshay from comment #0) > before this fix, that pseudo-code undesirably modified the value in the GPR! that's why temporary variables are always used, with bit-ranges on them, so that the RHS becomes inherently a copy. a <- RA[0:XLEN-1] can you please confirm that before committing this change you ran the *entire* test suite?
(In reply to Luke Kenneth Casson Leighton from comment #1) > (In reply to Jacob Lifshay from comment #0) > > > before this fix, that pseudo-code undesirably modified the value in the GPR! > > that's why temporary variables are always used, with bit-ranges on > them, so that the RHS becomes inherently a copy. well, now that's unnecessary! > > a <- RA[0:XLEN-1] > > can you please confirm that before committing this change you ran > the *entire* test suite? I did, before committing. you can also check ci, which passed: https://salsa.debian.org/Kazan-team/mirrors/openpower-isa/-/commit/4e701a851536bba6648779c183293ba75e7ea7b8
(In reply to Luke Kenneth Casson Leighton from bug #982 comment #100) > done, but i had to revert some code that jacob had added. > you'll need to re-run pywriter and pyfnwriter > > jacob can you please take a look as i have absolutely no clue > why copy_assign_rhs was added, it really should not have been, Please put that back, we need it. This bug I'm commenting on is the one I created when I added it and it explains why. > there are ways to write the pseudocode such that it is not > necessary to do (variable <- [0]*64 is the usual one) that doesn't actually fix the issues all the time.
basically, this is a workaround to get the by-value semantics that I expect the spec pseudocode authors expect, since python has by-reference semantics and assignments/function-calls in python just point at the original object instead of being copied by-value like is expected in HDL.
(In reply to Jacob Lifshay from comment #3) > Please put that back, we need it. i don't understand it so i can't. i had to revert it in an emergency to get sc operational, i apologise i have to leave it to you. we'll up the budget portion of bug #982 to you, but do make sure that the new TrapTestCase test_sc works.
(In reply to Luke Kenneth Casson Leighton from comment #5) > (In reply to Jacob Lifshay from comment #3) > > > Please put that back, we need it. > > i don't understand it so i can't. > > i had to revert it in an emergency to get sc operational, > i apologise i have to leave it to you. we'll up the > budget portion of bug #982 to you, but do make sure that the > new TrapTestCase test_sc works. repeating what I confirmed with luke in a private email: ok, I'll work on re-adding copy_assign_rhs and ensure the syscall tests still work. I'll work on this soon, probably later today.
(In reply to Jacob Lifshay from comment #6) > ok, I'll work on re-adding copy_assign_rhs and ensure the syscall tests > still work. > > I'll work on this soon, probably later today. Done, I pushed to programmerjake/readd-rhs-copy, all tests pass except for the ones I expect to fail (they started failing a long time ago). Since it works can I push to master? test failures: > FAILED src/openpower/decoder/isa/test_caller_syscall.py::TestSysCall::test - ... > FAILED src/openpower/sv/trans/test_pysvp64dis.py::SVSTATETestCase::test_20_cmp > FAILED src/openpower/sv/trans/test_pysvp64dis.py::SVSTATETestCase::test_36_extras_rldimi_ > FAILED src/openpower/sv/trans/test_pysvp64dis.py::SVSTATETestCase::test_37_extras_rldimi > FAILED src/openpower/sv/trans/test_pysvp64dis.py::SVSTATETestCase::test_36_extras_rldimi https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=c3fe6beac9acce833c3e140b967000814d422998 commit c3fe6beac9acce833c3e140b967000814d422998 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 18:18:51 2023 -0700 Revert "Revert "fix bug where pseudo-code assignments modify more than just the variable being assigned to"" we need copy_assign_rhs See https://bugs.libre-soc.org/show_bug.cgi?id=1066 This reverts commit bd3b54e83101217dc32da09083c6a3858fd7c600. commit 4780418b01cc894aa817074bd3aaa7facffa074f Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 18:17:20 2023 -0700 Revert "fix bug introduced by having to revert unauthorized addition of" we need copy_assign_rhs See https://bugs.libre-soc.org/show_bug.cgi?id=1066 This reverts commit 9dab88318a2938f14873804d83bf85ef9ae2fb93. commit da17d8d36e95bff174cee28eb316420c1c80b2b3 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 18:00:55 2023 -0700 skip broken test it wasn't obvious how to fix it, see https://bugs.libre-soc.org/show_bug.cgi?id=1193 commit 4280c97f2041ce4b61b183067e708c3cc7f9727a Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 17:49:04 2023 -0700 fill in manually verified expected state for TrapTestCase.case_2_kaivb_test based on the Programming Note on left side of PowerISA v3.1B page 1289 (1315) commit ad85f6ae361b87d4f705c48a6fb300ee5ce43c56 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 17:37:34 2023 -0700 format code
(In reply to Jacob Lifshay from comment #7) > > commit da17d8d36e95bff174cee28eb316420c1c80b2b3 > Author: Jacob Lifshay <programmerjake@gmail.com> > Date: Thu Oct 19 18:00:55 2023 -0700 > > skip broken test do not do that. it was not requested. you must learn to pay closer attention, and not attempt to do more than what is requested. it only wastes both your time and then mine askig you again and again to undo work that was never needed to be done, and has detrimental consequences. > it wasn't obvious how to fix it, see if i had said it was needed i would have said "and do test_rfid" > https://bugs.libre-soc.org/show_bug.cgi?id=1193 please close this bug as invalid once the skip is reverted, as unauthorized work. > commit 4280c97f2041ce4b61b183067e708c3cc7f9727a > Author: Jacob Lifshay <programmerjake@gmail.com> > Date: Thu Oct 19 17:49:04 2023 -0700 > > fill in manually verified expected state for > TrapTestCase.case_2_kaivb_test this is good but unnecessary work. KAIVB is not an Authorized addition to the Power ISA.
(In reply to Luke Kenneth Casson Leighton from comment #8) > (In reply to Jacob Lifshay from comment #7) > > > > commit da17d8d36e95bff174cee28eb316420c1c80b2b3 > > Author: Jacob Lifshay <programmerjake@gmail.com> > > Date: Thu Oct 19 18:00:55 2023 -0700 > > > > skip broken test > > do not do that. as mentioned in bug #1193 comment #3, I reverted it. > > > it wasn't obvious how to fix it, see > > if i had said it was needed i would have said "and do test_rfid" > > > https://bugs.libre-soc.org/show_bug.cgi?id=1193 > > please close this bug as invalid once the skip is reverted, as > unauthorized work. I disagree, it is a valid bug because the test fails. adding skip_case being unauthorized doesn't mean it isn't a problem that needs to eventually be fixed by removing or correcting the test and/or correcting the simulator. > > > commit 4280c97f2041ce4b61b183067e708c3cc7f9727a > > Author: Jacob Lifshay <programmerjake@gmail.com> > > Date: Thu Oct 19 17:49:04 2023 -0700 > > > > fill in manually verified expected state for > > TrapTestCase.case_2_kaivb_test > > this is good but unnecessary work. KAIVB is not an > Authorized addition to the Power ISA. yup, just fixing it so pytest will try to run the sc test.
(In reply to Luke Kenneth Casson Leighton from bug #982 comment #146) > (In reply to Jacob Lifshay from comment #142) > > > because programmerjake/readd-rhs-copy (bug #1066) hasn't been merged yet, > > do it if they pass on the branch. I ran the tests on the latest version before pushing, tests pass (except for the 6 tests I expect to fail): https://salsa.debian.org/Kazan-team/mirrors/openpower-isa/-/jobs/4830471 I'll merge to master in the morning (both double checking that's what you meant and letting ghostmansd have some time to merge without conflicting with my push).
(In reply to Jacob Lifshay from comment #10) > meant and letting ghostmansd have some time to merge without conflicting > with my push). he's using rebase (or, more to the point everyone is required to use rebase) so it is fine.
rebased, tested, and pushed to master: https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=a69f6426b866cf86a0ec8fb21dac58cb289cba2f;hp=49cd5029f5aa997d80bfcdd5dbaaa873913f8b93 commit a69f6426b866cf86a0ec8fb21dac58cb289cba2f Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 18:57:00 2023 -0700 Revert "skip broken test" requested by luke: https://bugs.libre-soc.org/show_bug.cgi?id=1193#c1 This reverts commit e0a4f19b2c90be84a77a4aa584c6d60e508d92f5. commit ca57c0af2e6851c58e30b2e67fce4416996f69ae Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 18:18:51 2023 -0700 Revert "Revert "fix bug where pseudo-code assignments modify more than just the variable being assigned to"" we need copy_assign_rhs See https://bugs.libre-soc.org/show_bug.cgi?id=1066 This reverts commit bd3b54e83101217dc32da09083c6a3858fd7c600. commit 6a816b8090d6b5ee6aa7d3caa09c2c1a2b087533 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 18:17:20 2023 -0700 Revert "fix bug introduced by having to revert unauthorized addition of" we need copy_assign_rhs See https://bugs.libre-soc.org/show_bug.cgi?id=1066 This reverts commit 9dab88318a2938f14873804d83bf85ef9ae2fb93. commit e0a4f19b2c90be84a77a4aa584c6d60e508d92f5 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 18:00:55 2023 -0700 skip broken test it wasn't obvious how to fix it, see https://bugs.libre-soc.org/show_bug.cgi?id=1193 commit 5bd2d42ff69d976f3405f3f74aaac855617d85fd Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 17:49:04 2023 -0700 fill in manually verified expected state for TrapTestCase.case_2_kaivb_test based on the Programming Note on left side of PowerISA v3.1B page 1289 (1315) commit d05435414de95ab24bd321db7cc5623edfc37117 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Thu Oct 19 17:37:34 2023 -0700 format code