Bug 1247 - fminmax/fminmax.: pseudocode clarification needed
Summary: fminmax/fminmax.: pseudocode clarification needed
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC All
: --- enhancement
Assignee: Jacob Lifshay
URL:
Depends on:
Blocks:
 
Reported: 2024-01-07 23:04 GMT by Dmitry Selyutin
Modified: 2024-01-09 08:45 GMT (History)
3 users (show)

See Also:
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:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Selyutin 2024-01-07 23:04:31 GMT
openpower/isa/fptrans.mdwn contains the following line:

    if (FPSCR.VE = 0) | ¬any_snan then (FRT) <- result

Meanwhile OPF 3.1B 1.3.2 contains the following statements:

    (x) means the contents of register x, where x is the
    name of an instruction field. For example, (RA)
    means the contents of register RA, and (FRA)
    means the contents of register FRA, where RA and
    FRA are instruction fields. Names such as LR and
    CTR denote registers, not fields, so parentheses
    are not used with them. Parentheses are also
    omitted when register x is the register into which
    the result of an operation is placed.

Due to the (FRT) notation used, the FRT register gets marked as belonging to read_regs subset. It must be clarified whether this was an intentional decision or just a typo.
Comment 1 Dmitry Selyutin 2024-01-07 23:05:57 GMT
In order to clarify. I assume this pseudocode should have been used (note the absence of parentheses):

    if (FPSCR.VE = 0) | ¬any_snan then FRT <- result
Comment 2 Jacob Lifshay 2024-01-07 23:23:25 GMT
(In reply to Dmitry Selyutin from comment #1)
> In order to clarify. I assume this pseudocode should have been used (note
> the absence of parentheses):
> 
>     if (FPSCR.VE = 0) | ¬any_snan then FRT <- result

that looks like a typo to me, feel free to fix it, do fix it at the same time in the wiki too since we're trying to keep those in-sync (don't just copy the whole pseudocode block, there are some intentional differences iirc):
https://git.libre-soc.org/?p=libreriscv.git;a=blob;f=openpower/sv/rfc/ls013.mdwn;h=6c3b0eb444c7ce902e6853b41221d05ff7aa4c61;hb=6a352d8538954c57a5af7d6bdf703747abbbb776#l216
Comment 3 Luke Kenneth Casson Leighton 2024-01-08 00:51:18 GMT
(In reply to Jacob Lifshay from comment #2)

> that looks like a typo to me, feel free to fix it, do fix it at the same
> time in the wiki too since we're trying to keep those in-sync (don't just
> copy the whole pseudocode block, there are some intentional differences
> iirc):
> https://git.libre-soc.org/?p=libreriscv.git;a=blob;f=openpower/sv/rfc/ls013.
> mdwn;h=6c3b0eb444c7ce902e6853b41221d05ff7aa4c61;
> hb=6a352d8538954c57a5af7d6bdf703747abbbb776#l216

jacob you need to take care of this, don't ask someone else to do it.
(Charter Requirement. see "guilt and merit")

and there cannot be *any* differences.

**ALL** RFC pseudocode **WILL** be destroyed and replaced with the
canonical source from openpower/isa/*.mdwn with an "include"
via a pandoc filter, pagereader.py and insndb.
see bug #1048 comment 24
https://bugs.libre-soc.org/show_bug.cgi?id=1048#c24
Comment 4 Jacob Lifshay 2024-01-08 00:57:14 GMT
(In reply to Luke Kenneth Casson Leighton from comment #3)
> jacob you need to take care of this, don't ask someone else to do it.
> (Charter Requirement. see "guilt and merit")

it's a trivial enough change that he can do it if it's blocking his progress, if not, i'll do it later.
> 
> and there cannot be *any* differences.

iirc it's mostly that the RFC has some new lines that improve formatting (by matching the pseudocode it's based off of). our pseudocode parser *couldn't* parse empty lines when I wrote that (i'd guess it still can't).
Comment 5 Luke Kenneth Casson Leighton 2024-01-08 01:01:28 GMT
(In reply to Dmitry Selyutin from comment #0)

> Due to the (FRT) notation used, the FRT register gets marked as belonging to
> read_regs subset. It must be clarified whether this was an intentional
> decision or just a typo.

how did this get past unit tests? it would have to have been
added explicitly to ISACaller and the parser, to cope with it.

this is effectively double-indexing/double-lookup.

jacob as you are the author of fptrans.mdwn it is your responsibility,
please do take care of it quickly so as not to hold Dmitry up.

also, Dmitry (and Jacob) remember always always always link to at least
one other bug, orphan bugs are insanely hard to find.
Comment 6 Luke Kenneth Casson Leighton 2024-01-08 01:04:14 GMT
(In reply to Jacob Lifshay from comment #4)
> (In reply to Luke Kenneth Casson Leighton from comment #3)

> iirc it's mostly that the RFC has some new lines that improve formatting

definitely do not do that, extremely important under no circumstance.

> parse empty lines when I wrote that (i'd guess it still can't).

(you massively complexified pagereader.py i had to remove all use of
regexes. pagereader.py has to be brutally simple and understandable).
Comment 7 Jacob Lifshay 2024-01-08 01:04:41 GMT
(In reply to Luke Kenneth Casson Leighton from comment #5)
> how did this get past unit tests? it would have to have been
> added explicitly to ISACaller and the parser, to cope with it.

nope, it's part of python's syntax, so it was probably there the whole time:
$ python3
>>> (a) = 1
>>> a
1
Comment 8 Jacob Lifshay 2024-01-09 08:04:00 GMT
https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=fa629ca6cacabc960926decc03156d367fcbc295

commit fa629ca6cacabc960926decc03156d367fcbc295
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Tue Jan 9 00:01:44 2024 -0800

    fptrans.mdwn: don't parenthesize register being assigned to
    
    Fixes: https://bugs.libre-soc.org/show_bug.cgi?id=1247
Comment 9 Luke Kenneth Casson Leighton 2024-01-09 08:45:33 GMT
(In reply to Jacob Lifshay from comment #7)
> (In reply to Luke Kenneth Casson Leighton from comment #5)
> > how did this get past unit tests? it would have to have been
> > added explicitly to ISACaller and the parser, to cope with it.
> 
> nope, it's part of python's syntax, so it was probably there the whole time:
> $ python3
> >>> (a) = 1
> >>> a
> 1

ahhh, doh :)

thanks for taking care of this