Bug 1205

Summary:	pifpstore.mdwn conflicts with fpstore.mdwn -- defines stfsu twice
Product:	Libre-SOC's first SoC	Reporter:	Jacob Lifshay <programmerjake>
Component:	Source Code	Assignee:	Jacob Lifshay <programmerjake>
Status:	RESOLVED FIXED
Severity:	blocker	CC:	libre-soc-bugs, lkcl, programmerjake, shriya.sharma
Priority:	---
Version:	unspecified
Hardware:	PC
OS:	Linux
See Also:	https://bugs.libre-soc.org/show_bug.cgi?id=1125 https://bugs.libre-soc.org/show_bug.cgi?id=1048
NLnet milestone:	---	total budget (EUR) for completion of task and all subtasks:	0
budget (EUR) for this task, excluding subtasks' budget:	0	parent task for budget allocation:
child tasks for budget allocation:		The table of payments (in EUR) for this task; TOML format:
Bug Depends on:
Bug Blocks:	1177

Description Jacob Lifshay 2023-11-06 01:51:04 GMT

while trying to figure out why the newly merged shriya_add_descriptions branch breaks tons of tests, I discovered that the post-increment files have the exact same instruction names as the standard pre-increment instructions, this causes the wrong op_stfsu to be called. until now, it didn't matter since the instructions were still identical copies, but now that they've changed to have post-increment semantics, it broke tests using any load/store update instructions.

Example test that broke:
src/openpower/decoder/isa/test_caller_svp64_fp.py::DecoderTestCase::test_fp_single_ldst

it runs `sv.stfsu *0, 16(*4)` with r4 == 0 and r5 == 8, but because it's running the wrong pseudo-code, it stores to address 0x0 and 0x8 instead of 0x10, 0x18 like it's supposed to.

Whoever merged that branch *needed* to have run tests first, and upon observing that it broke a giant pile of tests, *not merged it to master* until sufficient fixes have been applied.

For the test I looked at, the offending commit is (but that's probably only one of many similar breaking commits):

commit 801d87dbb8d9943cdc06251cd52119a94e03632c
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Tue Oct 17 10:06:18 2023 +0100

    update pifpstore.mdwn pseudocode which was a copy of fpstore.mdwn
    to be actually post-increment

So, we need to rename those instructions to the proper names.

Comment 1 Luke Kenneth Casson Leighton 2023-11-06 02:53:07 GMT

(In reply to Jacob Lifshay from comment #0)

> Whoever merged that branch *needed* to have run tests first, and upon

me. i'm in absolutely no position whatsoever to be able to do that, as i am
on the move routinely and will be on battery power only.  i'm collecting the dell
server tomorrow and *may* be able to set it up on the UPS and a 12v WIFI network
over the next week or so.

in the meantime i'll be counting on you and others in the team to catch things
like this and help resolve them.

commit 9fe4ddc04c9b8f30d6efc74ce0173d385eb87f1f (HEAD -> master, origin/master, origin/HEAD)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Mon Nov 6 02:46:09 2023 +0000

    rename pifpstore instructions, add "p" into names

Comment 2 Jacob Lifshay 2023-11-06 03:02:28 GMT

(In reply to Luke Kenneth Casson Leighton from comment #1)
> (In reply to Jacob Lifshay from comment #0)
> 
> > Whoever merged that branch *needed* to have run tests first, and upon
> 
> me. i'm in absolutely no position whatsoever to be able to do that

this is why we *need* CI, because you can look at CI if you can't or don't want to run tests locally. just push to a branch (preferably not master) and it'll run tests for you. (which is also why it's practically necessary for it to run shortly after pushing commits to whatever branch is being worked on, rather than waiting 24hr)

> , as i am
> on the move routinely and will be on battery power only.  i'm collecting the
> dell
> server tomorrow and *may* be able to set it up on the UPS and a 12v WIFI
> network
> over the next week or so.

nice!
> 
> in the meantime i'll be counting on you and others in the team to catch
> things
> like this and help resolve them.
> 
> commit 9fe4ddc04c9b8f30d6efc74ce0173d385eb87f1f (HEAD -> master,
> origin/master, origin/HEAD)
> Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
> Date:   Mon Nov 6 02:46:09 2023 +0000
> 
>     rename pifpstore instructions, add "p" into names

thanks, though I already made a commit fixing that (and all the other files) on the programmerjake/bug-1177 branch, I just hadn't pushed it yet (sorry):
https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=5a2cd34ab850aa388cd381ff73ea5bbc5c92cdce

Comment 3 Jacob Lifshay 2023-11-06 03:23:38 GMT

ok, I rebased and pushed to master:

https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=49cba08576f5efa398432acaa2d43ca6cd0d24eb

commit 49cba08576f5efa398432acaa2d43ca6cd0d24eb
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Sun Nov 5 18:20:42 2023 -0800

    fix instruction name conflicts

commit 3c6fa248c5fc4c7be792760ac6e22eb4b47b017e
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Sun Nov 5 18:18:08 2023 -0800

    remove lhaup from pifixedloadshift -- all pi-shift instructions are indexed X-Form rather than D-Form

the real lhaup is in pyfixedload.mdwn and wasn't removed.

commit 4c35231597f4400787c504373e58c24fa439196d
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Sun Nov 5 18:15:03 2023 -0800

    detect duplicate instructions