Bug 685 - Implement XLEN-ification for BCD instructions in base OpenPower ISA
Summary: Implement XLEN-ification for BCD instructions in base OpenPower ISA
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Specification (show other bugs)
Version: unspecified
Hardware: All All
: --- enhancement
Assignee: Jacob Lifshay
URL:
Depends on:
Blocks: 671
  Show dependency treegraph
 
Reported: 2021-09-06 16:19 BST by Jacob Lifshay
Modified: 2021-10-20 21:02 BST (History)
3 users (show)

See Also:
NLnet milestone: NLNet.2019.10.046.Standards
total budget (EUR) for completion of task and all subtasks: 350
budget (EUR) for this task, excluding subtasks' budget: 350
parent task for budget allocation: 242
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
jacob={amount=350,paid=2021-10-20}


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Lifshay 2021-09-06 16:19:01 BST
https://bugs.libre-soc.org/show_bug.cgi?id=671#c70
Comment 1 Jacob Lifshay 2021-09-07 08:05:34 BST
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.
Comment 2 Jacob Lifshay 2021-09-07 08:09:31 BST
(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.
Comment 3 Luke Kenneth Casson Leighton 2021-09-07 12:25:57 BST
(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.
Comment 4 Luke Kenneth Casson Leighton 2021-09-07 12:37:21 BST
test_caller_bcd.py passed in the xlen branch, and once cherry-picked.
git pushed to master. running test_issuer.py to confirm.
Comment 5 Jacob Lifshay 2021-09-07 18:19:42 BST
(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.
Comment 6 Luke Kenneth Casson Leighton 2021-09-07 18:46:10 BST
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.
Comment 7 Jacob Lifshay 2021-09-07 19:13:08 BST
(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.
Comment 8 Jacob Lifshay 2021-09-16 22:07:02 BST
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.
Comment 9 Luke Kenneth Casson Leighton 2021-09-17 17:52:42 BST
confirmed working, still must cherrypick (done)
 3415  git cherry-pick 096b8d
 3416  git cherry-pick 59c0e0
 3417  git cherry-pick 2a7ed3
Comment 10 Jacob Lifshay 2021-09-23 02:45:06 BST
changing milestone to match parent task for budget allocation