https://bugs.libre-soc.org/show_bug.cgi?id=671#c70
I've basically completed adding XLEN support to the bcd instructions, I also added a new test suite that actually fully tests the bcd instructions, since the old test suite didn't support checking if invalid BCD inputs converted correctly. The new test suite uses pia as the reference implementation, since that makes it easy to test against a working Power9 system. I ended up rewriting the spec pseudo-code for addg6s since my attempted XLEN-ification made the already complex code even more horrendous (something like 15 lines!), the rewritten form is waay simpler and is nearly trivial to XLEN-ify (5 lines). New code: sum <- (0b0000 || (RA)) + (0b0000 || (RB)) carries <- sum ^ (0b0000 || (RA)) ^ (0b0000 || (RB)) ones <- [0b0001] * (XLEN / 4) nibbles_need_sixes <- ¬carries[0:XLEN-1] & ones RT <- nibbles_need_sixes * 0b0110 I'll mark this complete after someone else can replicate the tests working (you'll need the latest pia), assuming no one complains about the new pseudo-code algorithm.
(In reply to Jacob Lifshay from comment #1) > the old test suite didn't support checking if invalid BCD inputs converted > correctly. The old test suite uses reference code based on matching against a table that goes from 000 to 999, so the reference code will fail if the input is something like 0xA7E since that isn't in the table. Pia uses the boolean expressions from the OpenPower spec, so can handle all binary input values.
(In reply to Jacob Lifshay from comment #2) > (In reply to Jacob Lifshay from comment #1) > > the old test suite didn't support checking if invalid BCD inputs converted > > correctly. > > The old test suite uses reference code based on matching against a table > that goes from 000 to 999, so the reference code will fail if the input is > something like 0xA7E since that isn't in the table. Pia uses the boolean > expressions from the OpenPower spec, so can handle all binary input values. just going through things. https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=338e538b7148554adb64025ac6cfe8e274f6d8c4 looks good. https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=096b8d60b6500ec8af98c2442ebb0f31066b0bcb this depends on pia. as i explained, pia has to go, therefore adding more dependence on it means more work down the line to do. can you please therefore remove the dependence on pia. it doesn't matter if it's more "work". whatever is in pia needs to be rewritten in pure python, to make it long-term portable with no high-maintenance-cost "surprises", and be something that 30% of the world's programmers can work on https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=59c0e03a15e3c62603e91e23ac775de42a05d8e7 eurr! intriguing :) https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=d05da12e8e726e2de36ff468158a1371ba9ab917 looks waaay better. the general practice in the Power ISA document is to use (obscure and) short variable names. i'm sure they'll get over the use of meaningful ones. i'll re-run test_caller_bcd.py to make sure it passes (with the new XLEN-based pseudo-code). test_caller_bcd_long.py i will wait until the dependence on pia has been removed.
test_caller_bcd.py passed in the xlen branch, and once cherry-picked. git pushed to master. running test_issuer.py to confirm.
(In reply to Luke Kenneth Casson Leighton from comment #3) > https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff; > h=096b8d60b6500ec8af98c2442ebb0f31066b0bcb > > this depends on pia. as i explained, pia has to go, therefore > adding more dependence on it means more work down the line to > do. I think we should keep the dependence on pia, since if/when we replace it, I'm planning on ending up with something that has the same Python API, so it'll be a drop-in replacement. Until then, we *need* something that is tested against a native Power9 cpu, pia is by far the easiest route to do that. The additional work required to port pia caused by this I estimate to be around 30min -- see the pia commit: https://git.libre-soc.org/?p=power-instruction-analyzer.git;a=commitdiff;h=95fdd1c4edbd91c0a02b772ba02aa2045101d2b0 > can you please therefore remove the dependence on pia. that would be several days more work, to get testing against a power9 cpu, that code would likely just get thrown out again if/when pia is ported to non-Rust. If we're going to invest that time it would be better to spend it writing a pia replacement. So, can you please just accept the new tests for now, and if we're going to spend a bunch of extra time, let me spend it rewriting pia in Python or C or something.
the answer still remains no. the line has to be drawn, and it was drawn when dmitry couldn't (and still hasn't) been able to install pia. if this unit test depends on pia *dmitry cannot run it*. whatever work has to be done to get this test to be in python-only working right now is perfectly fine and 100% preferable to the situation of perpetuating further critical dependence on pia. i really mean it: NO using pia. the cost has been demonstrated to be too high. do whatever you need as quickly as you can, in pure python only: don't consider "designing an API" just get it done, fast. i'll back you with an appropriate budget and think about ways to justify getting a larger one into the (new) simulator Grant proposal submitted last month.
(In reply to Luke Kenneth Casson Leighton from comment #6) > the answer still remains no. the line has to be drawn, and it was drawn when > dmitry couldn't (and still hasn't) been able to install pia. oh, didn't realize he still couldn't install pia... > do whatever you need as quickly as you can, in pure python only: > don't consider "designing an API" just get it done, fast. > i'll back you with an appropriate budget and think about ways > to justify getting a larger one into the (new) simulator > Grant proposal submitted last month. How about I start working on porting pia to python? the original test is good enough for a stop-gap that dmitry can use meanwhile.
I added a mock implementation of pia to the new test for use until pia is ported to python, that should be the last step needed for this bug. lkcl, once you cherrypick the commit, please close this bug.
confirmed working, still must cherrypick (done) 3415 git cherry-pick 096b8d 3416 git cherry-pick 59c0e0 3417 git cherry-pick 2a7ed3
changing milestone to match parent task for budget allocation