Bug 1057 - split out all int/fp min/max ops into their own RFC ls013
Summary: split out all int/fp min/max ops into their own RFC ls013
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Specification (show other bugs)
Version: unspecified
Hardware: Other Linux
: --- enhancement
Assignee: Jacob Lifshay
URL: https://libre-soc.org/openpower/sv/rf...
Depends on: 910 915
Blocks: 1230
  Show dependency treegraph
 
Reported: 2023-04-13 19:43 BST by Jacob Lifshay
Modified: 2024-03-01 00:16 GMT (History)
4 users (show)

See Also:
NLnet milestone: NLnet.2022-08-051.OPF
total budget (EUR) for completion of task and all subtasks: 2500
budget (EUR) for this task, excluding subtasks' budget: 2500
parent task for budget allocation: 1009
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
lkcl = { amount = 800, submitted = 2023-12-03, paid = 2023-12-19 } ghostmansd = { amount = 500, submitted = 2024-01-18, paid = 2024-01-29 } [jacob] amount = 1200 submitted = 2024-02-09 paid = 2024-02-29


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Lifshay 2023-04-13 19:43:00 BST
* DONE: merge fminmax(s) and minmax into 2 and 1 XO respectively
* DONE: add new MM-form to fields.txt
* DONE: reallocate fminmax in power_trans_ops.mdwn
* DONE: reallocate fminmax in openpower-isa.git
* DONE: should we just remove fminmaxs[.] since it's redundant? comment #28
* DONE: reallocate minmax in openpower-isa.git
* DONE: write fminmax pseudo-code
* DONE: add fminmax to simulator
* DONE: add minmax to simulator
* DONE: Q: should minmax use RA_OR_ZERO in the csv?
           it uses (RA|0) in the pseudo-code...
        A: Always yes
        https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=6626fc852
* DONE: add/modify unit tests for fminmax
  * DONE: modify existing unit tests for fminmax
  * DONE: add more unit tests for fminmax, testing all the corner cases
* DONE: update binutils for new encodings bug #1068
* add/modify unit tests for minmax
  * DONE: modify existing unit tests for minmax
  * MOVED: bug #1230
           add more unit tests for minmax, testing for RA=0 and *w variants

we will likely need them for vector reductions before we submit the a/v and transcendentals RFCs.
Comment 1 Jacob Lifshay 2023-04-13 19:48:55 BST
full list:
fminnum08(s)
fmaxnum08(s)
fmin19(s)
fmax19(s)
fminnum19(s)
fmaxnum19(s)
fminc(s)
fmaxc(s)
fminmagnum08(s)
fmaxmagnum08(s)
fminmag19(s)
fmaxmag19(s)
fminmagnum19(s)
fmaxmagnum19(s)
fminmagc(s)
fmaxmagc(s)
mins
maxs
minu
maxu
Comment 2 Luke Kenneth Casson Leighton 2023-04-13 22:39:00 BST
(In reply to Jacob Lifshay from comment #0)
> we will likely need them for vector reductions before we submit the a/v and
> transcendentals RFCs.

and also min/max combined with inner product matrix equals
"Warshall Transitive Closure". yep let's do it.

(In reply to Jacob Lifshay from comment #1)
> full list:
> fminnum08(s)
> fmaxnum08(s)

making me nervous the quantity, can we do the same trick
of a mode-operand and get them down in quantity?
the fp op table likely has the mode bits already.


> mins
> maxs
> minu
> maxu

happy with these.

important justification, that using VSX may have
different meaning (SVP64/VSX) so it is *really*
crucial to have SVP64/SFFS ops.
Comment 3 Jacob Lifshay 2023-04-15 02:49:54 BST
filled out ls013:
https://git.libre-soc.org/?p=libreriscv.git;a=shortlog;h=70dda033b8caf1bcfcd53302b6e04481a5b8b787

commit 70dda033b8caf1bcfcd53302b6e04481a5b8b787
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Fri Apr 14 18:47:16 2023 -0700

    add lkcl's justification for fmin/fmax

commit 763a74315adf9b265ce681f0ee6da24a176a08e7
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Fri Apr 14 18:46:56 2023 -0700

    word wrap

commit 61d4047a5c4add0edb6068d6f26be2a0a2005bee
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Fri Apr 14 18:42:09 2023 -0700

    fill out ls013 min/max fmin/fmax

(In reply to Luke Kenneth Casson Leighton from comment #2)
> (In reply to Jacob Lifshay from comment #1)
> > full list:
> > fminnum08(s)
> > fmaxnum08(s)
> 
> making me nervous the quantity, can we do the same trick
> of a mode-operand and get them down in quantity?
> the fp op table likely has the mode bits already.

I had to reorganize them somewhat...

> important justification, that using VSX may have
> different meaning (SVP64/VSX) so it is *really*
> crucial to have SVP64/SFFS ops.

added with a TODO(lkcl) so you can elaborate.
Comment 4 Luke Kenneth Casson Leighton 2023-04-19 18:06:42 BST
(In reply to Jacob Lifshay from comment #3)
> filled out ls013:

excellent. sorry missed this

> I had to reorganize them somewhat...

looks good.
 
> > important justification, that using VSX may have
> > different meaning (SVP64/VSX) so it is *really*
> > crucial to have SVP64/SFFS ops.
> 
> added with a TODO(lkcl) so you can elaborate.

got it. looking for a place to put it.

observations:

can you keep instruction spec layout to exactly the order in
power isa spec?

    title, form, syntaxes, pseudocode, specials, english words, examples

is the typical order.  you have words before specials.


second observation: Rc=1 is missing from fmin/max which is important.
means moving to a different Form.

related: X-Form is *not* appropriate. okok it is sort-of because
these are same instructions but let's not confuse matters.  X is
O from 21 thru 30.

A-Form would be better:

 194 
 195 # 1.6.17 A-FORM
 196     |0     |6     |11      |16     |21      |26    |31 |
 197     | PO   |  FRT |   FRA  | FRB   |   FRC  |   XO |Rc |
 198     | PO   |  FRT |   FRA  | FRB   |  FMM / |   XO |Rc |
Comment 5 Luke Kenneth Casson Leighton 2023-04-19 19:37:25 BST
hm i think a table for int mmax s/u and down to 1 instruction
is sensible, it will keep the message clear to keep the decoder
simple.
Comment 6 Luke Kenneth Casson Leighton 2023-04-19 19:39:08 BST
(In reply to Luke Kenneth Casson Leighton from comment #5)
> hm i think a table for int mmax s/u and down to 1 instruction
> is sensible, it will keep the message clear to keep the decoder
> simple.

Z23-Form.  sm bit 0 min/max sm bit 1 sign/uns

 270 # 1.6.27 Z23-FORM
 271     |0     |6     |11    |15 |16     |21 |23    |31 |

 275     | PO   |  RT  |   RA     |   RB  |sm |   XO |Rc |
 276     | PO   |  RT  |   RA     |   RB  |CY |   XO |Rc |
Comment 7 Jacob Lifshay 2023-04-19 19:46:13 BST
while we're changing stuff, adding min[u]w/max[u]w would be useful, since lots of programs often use 32-bit types, and PowerISA has *w variants where they'd be different than the lower 32-bits of 64-bit variants.
Comment 8 Luke Kenneth Casson Leighton 2023-04-19 19:49:00 BST
(In reply to Jacob Lifshay from comment #7)
> while we're changing stuff, adding min[u]w/max[u]w would be useful, since
> lots of programs often use 32-bit types, and PowerISA has *w variants where
> they'd be different than the lower 32-bits of 64-bit variants.

hmmm.... what other ISAs have them? i'm getting concerned about the number
of *w variants.  are those present in VSX?
Comment 9 Jacob Lifshay 2023-04-19 20:03:19 BST
(In reply to Luke Kenneth Casson Leighton from comment #8)
> (In reply to Jacob Lifshay from comment #7)
> > while we're changing stuff, adding min[u]w/max[u]w would be useful, since
> > lots of programs often use 32-bit types, and PowerISA has *w variants where
> > they'd be different than the lower 32-bits of 64-bit variants.
> 
> hmmm.... what other ISAs have them? i'm getting concerned about the number
> of *w variants.  are those present in VSX?

arm has smax on 32/64-bit scalar registers and presumably also umax/smin/umin.
arm also has atomic fetch-and-min/max
arm and Power also have unsigned/signed simd min/max. VSX has no scalar integer min/max.
Comment 10 Jacob Lifshay 2023-04-19 20:05:20 BST
(In reply to Jacob Lifshay from comment #9)
> (In reply to Luke Kenneth Casson Leighton from comment #8)
> > (In reply to Jacob Lifshay from comment #7)
> > > while we're changing stuff, adding min[u]w/max[u]w would be useful, since
> > > lots of programs often use 32-bit types, and PowerISA has *w variants where
> > > they'd be different than the lower 32-bits of 64-bit variants.
> > 
> > hmmm.... what other ISAs have them? i'm getting concerned about the number
> > of *w variants.  are those present in VSX?
> 
> arm has smax on 32/64-bit scalar registers and presumably also
> umax/smin/umin.
> arm also has atomic fetch-and-min/max
> arm and Power also have unsigned/signed simd min/max. VSX has no scalar
> integer min/max.

x86 has fp/int simd min/max but only has fp scalar min/max.
Comment 11 Luke Kenneth Casson Leighton 2023-04-19 20:18:53 BST
(In reply to Jacob Lifshay from comment #10)

> x86 has fp/int simd min/max but only has fp scalar min/max.

i meant has VSX *elements* got minw. answer is no. section 6.9.2
p379 Public v3.1.  i am tempted
to suggest that programmers may use extsw and then the full
64-bit min/max.

adding functionality that no other ISA has is risky, it means we have
no historic or technical justification.
Comment 12 Jacob Lifshay 2023-04-19 20:24:03 BST
(In reply to Luke Kenneth Casson Leighton from comment #11)
> (In reply to Jacob Lifshay from comment #10)
> 
> > x86 has fp/int simd min/max but only has fp scalar min/max.
> 
> i meant has VSX *elements* got minw. answer is no. section 6.9.2
> p379 Public v3.1.  i am tempted
> to suggest that programmers may use extsw and then the full
> 64-bit min/max.
> 
> adding functionality that no other ISA has is risky, it means we have
> no historic or technical justification.

I just pointed out arm has 32/64-bit scalar min/max, if it's that much of an issue we could declare min[u]w/max[u]w unvectorizable but imho that's a terrible idea, we should have both scalar (what I think is the main justification) and svp64 vector min[u]w/max[u]w (just to be orthogonal).
Comment 13 Jacob Lifshay 2023-04-19 20:31:17 BST
vector minuw is useful as a replacement for the sequence:
u64x4 -> u32x4 -> minu -> u64x4
or other min-at-half-width ops.
note we need half-width compare anyway for vector cmp[l]w, so having min[u]w/max[u]w mostly just means adding an output mux assuming compare is in the same ALU as 2-in 1-out gpr ops.
Comment 14 Jacob Lifshay 2023-04-20 01:22:29 BST
I think we shouldn't call it IMM since that sounds exactly like immediate, so I'll pick MMM (Min/Max Mode) for now. FMM can stay since it isn't as ambiguous.

I think rather than using A form and wasting extra bits we should have a new form for min/max:
    MM-FORM:
    |0    |6    |11   |16   |21   |24 |25  |31  |
    | PO  | FRT | FRA | FRB | FMM     | XO | Rc |
    | PO  | RT  | RA  | RB  | MMM | / | XO | Rc |

This way both integer and fminmax can, if desired, fit in the space needed by just fminmax before. fminmaxs is still separate.

What do you think?
Comment 15 Jacob Lifshay 2023-04-20 01:49:44 BST
https://git.libre-soc.org/?p=libreriscv.git;a=shortlog;h=919776f50ae62b791e61c689ca487f82eabc4f82

I applied the new MMM naming to the integer minmax instruction description since I was rewriting it anyway, but left it as IMM everywhere else until luke responds to comment #14

commit 919776f50ae62b791e61c689ca487f82eabc4f82
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Apr 19 17:45:44 2023 -0700

    merge integer min/max instructions and correct section ordering

commit d2c0ea5a9f292dbe80a92a6035c25f8b79fa628a
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Apr 19 17:45:11 2023 -0700

    fix typo

commit 592846c70beee4020508cb24f18560493db7281a
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Apr 19 17:44:32 2023 -0700

    expand out integer min/max mode table

commit 938cf473641d45090671bafdd7d9add176f5fd4a
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Wed Apr 19 17:41:36 2023 -0700

    fix fminmax special registers altered
Comment 16 Luke Kenneth Casson Leighton 2023-04-20 04:20:53 BST
(In reply to Jacob Lifshay from comment #14)
> I think we shouldn't call it IMM since that sounds exactly like immediate,
> so I'll pick MMM (Min/Max Mode) for now. FMM can stay since it isn't as
> ambiguous.

ack.
 
>     MM-FORM:
>     |0    |6    |11   |16   |21   |24 |25  |31  |
>     | PO  | FRT | FRA | FRB | FMM     | XO | Rc |
>     | PO  | RT  | RA  | RB  | MMM | / | XO | Rc |

nice.
 
> This way both integer and fminmax can, if desired, fit in the space needed
> by just fminmax before. fminmaxs is still separate.

yes.
 
> What do you think?

yep like it.
Comment 17 Luke Kenneth Casson Leighton 2023-04-20 13:58:36 BST
(In reply to Jacob Lifshay from comment #13)
> vector minuw is useful as a replacement for the sequence:
> u64x4 -> u32x4 -> minu -> u64x4
> or other min-at-half-width ops.

yes that looks terrible.

> note we need half-width compare anyway for vector cmp[l]w, 

click. yes. ok am in.  so that's a 3-bit Mode.

(In reply to Jacob Lifshay from comment #12)

> I just pointed out arm has 32/64-bit scalar min/max, 

missed that sorry

> if it's that much of an
> issue we could declare min[u]w/max[u]w unvectorizable 

blech no.

> but imho that's a
> terrible idea, we should have both scalar (what I think is the main
> justification) and svp64 vector min[u]w/max[u]w (just to be orthogonal).

yes. orthogonal "wins" evrry time.
Comment 18 Luke Kenneth Casson Leighton 2023-04-20 16:37:46 BST
eek forgot min/max overflow bug #915
and bug #910
Comment 19 Jacob Lifshay 2023-04-21 01:42:18 BST
https://git.libre-soc.org/?p=libreriscv.git;a=shortlog;h=79c462ecb4835fdd1602ea510608fe9205f75a8c

commit 79c462ecb4835fdd1602ea510608fe9205f75a8c
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Apr 20 17:39:34 2023 -0700

    fill in instruction forms

commit 443734d54a5558d2f2d34ad451b83daf322d62bd
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Apr 20 17:38:50 2023 -0700

    change fminmax[s][.] to MM-form

commit 146c8b9b1cb2a755345218c10055e38e1740a102
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Apr 20 17:37:48 2023 -0700

    rename IMM integer min/max mode table -> MMM

commit 44437f9ee605322b2b383f537d0549325a7e0fab
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Apr 20 17:35:37 2023 -0700

    reformat list

commit 4952e6574cb09755883e7d00932de7dd5032d653
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Apr 20 17:33:55 2023 -0700

    fill in ls013 motivation
Comment 20 Jacob Lifshay 2023-04-21 03:38:04 BST
I reallocated the draft fminmax insns in power_trans_ops.mdwn since there wasn't space where they were without needing to reallocate most of the other fptrans ops.

commit 2aecbce83e1658c652bb04f7daca124239d05edd
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Thu Apr 20 19:31:53 2023 -0700

    move fminmax[s] to XO ....0 10000

I still need to adjust them in the simulator and likewise reallocate integer minmax since afaict there isn't space in PO22 without reallocating a bunch of stuff.
Comment 21 Jacob Lifshay 2023-04-21 03:44:22 BST
I added a todo list in the top comment.

(In reply to Jacob Lifshay from comment #20)
> I still need to adjust them in the simulator and likewise reallocate integer
> minmax since afaict there isn't space in PO22 without reallocating a bunch
> of stuff.

to be clear, I'm planning on allocating the draft opcode somewhere other than PO22 so we don't have to move anything else.
Comment 22 Luke Kenneth Casson Leighton 2023-04-21 08:52:09 BST
(In reply to Jacob Lifshay from comment #21)
> I added a todo list in the top comment.

good idea

> to be clear, I'm planning on allocating the draft opcode somewhere other
> than PO22 so we don't have to move anything else.

yes please that table took ages and is really fragile/at-limit.
fp goes in 59 and 63 (i think).  minmax have to take care, put in
19 for now.
Comment 23 Jacob Lifshay 2023-04-25 04:49:31 BST
https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;h=744fc36ecedda983fd67019b9642a20454d33535

commit 744fc36ecedda983fd67019b9642a20454d33535
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Apr 24 20:38:35 2023 -0700

    Extended Mnemonics come *after* Special Registers Altered
    
    see addpcis in PowerISA v3.1B
Comment 24 Jacob Lifshay 2023-04-25 07:56:46 BST
https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=e3b46c6ec7911d08b8b0c4cb3c286c3786dae2ef

commit e3b46c6ec7911d08b8b0c4cb3c286c3786dae2ef
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Apr 24 23:49:19 2023 -0700

    replace min/max[su][.] with minmax[.]

commit 56697594f5f676339780d113a0be5ecf947c09e5
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Apr 24 23:47:20 2023 -0700

    add unofficial and comment2 columns to minor_19.csv

commit 3ee28dbe043cdf1b6238b2abcaee098471a0aac3
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Apr 24 23:45:18 2023 -0700

    add MM-form


https://git.libre-soc.org/?p=libreriscv.git;a=shortlog;h=f360b3c9044b7fb3a3c2b5c80f147e3ae9ffcb1f

commit f360b3c9044b7fb3a3c2b5c80f147e3ae9ffcb1f
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Apr 24 23:43:02 2023 -0700

    allocate draft opcode for minmax

commit 3687b7b07fab4330fce32107d1abbf97db3b8e2a
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Apr 24 23:42:11 2023 -0700

    add mentions of ls013

commit 52434df9a3b7125137fe786821a598575c9f5edc
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Apr 24 23:40:35 2023 -0700

    fix minmax pseudo-code -- CR0 must not have lt/gt swapped
Comment 25 Luke Kenneth Casson Leighton 2023-04-25 08:42:41 BST
(In reply to Jacob Lifshay from comment #24)
> https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;
> h=e3b46c6ec7911d08b8b0c4cb3c286c3786dae2ef
> 
> commit e3b46c6ec7911d08b8b0c4cb3c286c3786dae2ef
> Author: Jacob Lifshay <programmerjake@gmail.com>
> Date:   Mon Apr 24 23:49:19 2023 -0700
> 
>     replace min/max[su][.] with minmax[.]

looks really good

> commit 56697594f5f676339780d113a0be5ecf947c09e5
> Author: Jacob Lifshay <programmerjake@gmail.com>
> Date:   Mon Apr 24 23:47:20 2023 -0700
> 
>     add unofficial and comment2 columns to minor_19.csv

hmmm be *really* careful. don't take any shortcuts: always
run the *entire* test suite if you make these kinds of
severely-low-level changes.
> commit 52434df9a3b7125137fe786821a598575c9f5edc
> Author: Jacob Lifshay <programmerjake@gmail.com>
> Date:   Mon Apr 24 23:40:35 2023 -0700
> 
>     fix minmax pseudo-code -- CR0 must not have lt/gt swapped

ah yeah i always get those wrong

@@ -180,7 +180,7 @@ class AVTestCase(TestAccumulatorBase):
         e.intregs[3] = min(e.intregs[1], e.intregs[2])
-        e.crregs[0] = 0x4
+        e.crregs[0] = 0x8  # r1 <u r2
         self.add_case(Program(lst, bige
Comment 26 Jacob Lifshay 2023-04-25 08:58:44 BST
(In reply to Luke Kenneth Casson Leighton from comment #25)
> (In reply to Jacob Lifshay from comment #24)
(edit: trimmed context -- i forgot
> > commit 56697594f5f676339780d113a0be5ecf947c09e5
> > Author: Jacob Lifshay <programmerjake@gmail.com>
> > Date:   Mon Apr 24 23:47:20 2023 -0700
> > 
> >     add unofficial and comment2 columns to minor_19.csv
> 
> hmmm be *really* careful. don't take any shortcuts: always
> run the *entire* test suite if you make these kinds of
> severely-low-level changes.

i ran the tests after writing all the code but before splitting it into commits

> > commit 52434df9a3b7125137fe786821a598575c9f5edc
> > Author: Jacob Lifshay <programmerjake@gmail.com>
> > Date:   Mon Apr 24 23:40:35 2023 -0700
> > 
> >     fix minmax pseudo-code -- CR0 must not have lt/gt swapped
> 
> ah yeah i always get those wrong

yup, i needed to do the CR0 computation *before* swapping a/b for the min/max
> 
> @@ -180,7 +180,7 @@ class AVTestCase(TestAccumulatorBase):
>          e.intregs[3] = min(e.intregs[1], e.intregs[2])
> -        e.crregs[0] = 0x4
> +        e.crregs[0] = 0x8  # r1 <u r2
>          self.add_case(Program(lst, bige

that was because the CR0 = cmp(RA, RB) changes had previously only been applied to maxs, now, because of the new pseudo-code, they're applied to all minmax. modes.
Comment 27 Jacob Lifshay 2023-04-26 02:46:57 BST
https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;h=8967ddf3e3fbcc16b0dfe07dfd68453b2665e661

commit 8967ddf3e3fbcc16b0dfe07dfd68453b2665e661
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Tue Apr 25 18:44:18 2023 -0700

    add fminmax pseudo-code
Comment 28 Jacob Lifshay 2023-04-26 02:52:52 BST
I realized that fminmaxs[.] is redundant since it always produces the exact same results as fminmax[.] (for scalar at least), should we just remove it?

It produces the exact same results because min/max is independent of precision/range and has no round off error, it behaves identically for f32/f64/f32-in-f64/etc. the only part that varies is which bits it reads from and sets for quieting sNaNs, and the changes caused by SINGLE/DOUBLE (only relevant for not-really-f32 in f32-in-f64), which is only relevant for f32, not f64/f32-in-f64.

Luke, what do you think?
Comment 29 Jacob Lifshay 2023-04-26 03:03:55 BST
(In reply to Jacob Lifshay from comment #28)
> It produces the exact same results because min/max is independent of
> precision/range and has no round off error, it behaves identically for
> f32/f64/f32-in-f64/etc. the only part that varies is which bits it reads
> from and sets for quieting sNaNs, and the changes caused by SINGLE/DOUBLE
> (only relevant for not-really-f32 in f32-in-f64), which is only relevant for
> f32, not f64/f32-in-f64.

clarification, "only relevant for f32" is referring only to which bits are read/set (basically f32's exponent/mantissa fields are in a different place), I added the SINGLE/DOUBLE part after and forgot to update the rest of the sentence.
Comment 30 Luke Kenneth Casson Leighton 2023-04-26 05:27:36 BST
(In reply to Jacob Lifshay from comment #28)
> I realized that fminmaxs[.] is redundant since it always produces the exact
> same results as fminmax[.] (for scalar at least), should we just remove it?

ahh... oh! interesting. did IBM spot this in other min/max instructions?
Comment 31 Luke Kenneth Casson Leighton 2023-04-26 05:38:34 BST
 6.10.2 Vector Floating-Point Maximum/ Minimum Instructions  . . . 424

these are FP32 only

MnemonicInstruction NamePage 
xsmaxcdpVSX Scalar Maximum Type-C Double-Precision736 
xsmaxcqpVSX Scalar Maximum Type-C Quad-Precision738 
xsmaxdpVSX Scalar Maximum Double-Precision734 
xsmaxjdpVSX Scalar Maximum Type-J Double-Precision739 
xsmincdpVSX Scalar Minimum Type-C Double-Precision743 
xsmincqpVSX Scalar Minimum Type-C Quad-Precision745 
xsmindpVSX Scalar Minimum Double-Precision741 
xsminjdpVSX Scalar Minimum Type-J Double-Precision746 
Table 29.VSX Scalar BFP Maximum/Minimum Instructions

looks like they do but more because SIMD. and type J and C
whatever those are?
Comment 32 Jacob Lifshay 2023-04-26 05:54:32 BST
(In reply to Luke Kenneth Casson Leighton from comment #31)
>  6.10.2 Vector Floating-Point Maximum/ Minimum Instructions  . . . 424
> 
> these are FP32 only
> 
> MnemonicInstruction NamePage 
> xsmaxcdpVSX Scalar Maximum Type-C Double-Precision736 
> xsmaxcqpVSX Scalar Maximum Type-C Quad-Precision738 
> xsmaxdpVSX Scalar Maximum Double-Precision734 
> xsmaxjdpVSX Scalar Maximum Type-J Double-Precision739 
> xsmincdpVSX Scalar Minimum Type-C Double-Precision743 
> xsmincqpVSX Scalar Minimum Type-C Quad-Precision745 
> xsmindpVSX Scalar Minimum Double-Precision741 
> xsminjdpVSX Scalar Minimum Type-J Double-Precision746 
> Table 29.VSX Scalar BFP Maximum/Minimum Instructions
> 
> looks like they do but more because SIMD. and type J and C
> whatever those are?

type C is basically fminc/fmaxc
type J is IEEE 754-2019 minimum/maximum aka. fmin19/fmax19 -- J because that's Java's Math.max
Comment 33 Luke Kenneth Casson Leighton 2023-04-26 11:43:16 BST
(In reply to Jacob Lifshay from comment #32)

> type C is basically fminc/fmaxc
> type J is IEEE 754-2019 minimum/maximum aka. fmin19/fmax19 -- J because
> that's Java's Math.max

ok that makes sense.

ok so yes agreed no point doing s/d just d because all the VSX instructions are "packed" format, there are no VSX equivalent of
"s" fp32-in-a-64bit-reg instructions.  which is great.  i'll
kick them off the csv list for ls012.
Comment 34 Luke Kenneth Casson Leighton 2023-04-26 14:39:23 BST
binutils needs updating with the new definitions for minmax/fminmax
Comment 35 Jacob Lifshay 2023-05-07 08:56:56 BST
added a todo item for "should minmax use RA_OR_ZERO in the csv?"
https://libre-soc.org/irclog/%23libre-soc.2023-05-07.log.html#t2023-05-07T08:49:18
Comment 36 Jacob Lifshay 2023-07-17 23:57:35 BST
I updated openpower-isa.git to use the new fminmax instruction, and removed fminmaxs while I was at it. We still need more fminmax tests, since the current tests mostly just check for inputs that are only 1.0, which isn't particularly useful for anything other than a smoke test.


https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=1b158610f81ed633d2b5abd703b18b9712a6cd65

commit 1b158610f81ed633d2b5abd703b18b9712a6cd65
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Jul 17 15:49:17 2023 -0700

    update to use new fminmax instruction


https://git.libre-soc.org/?p=libreriscv.git;a=shortlog;h=da9125eead675307b373e33f26a4095af326fe4a

commit da9125eead675307b373e33f26a4095af326fe4a
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Jul 17 15:42:12 2023 -0700

    change wording to be more clear

commit 87ac057f41ec51e584096e3af92a342f2563f47e
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Jul 17 15:41:00 2023 -0700

    change pseudocode to match openpower-isa.git

commit f5a9262f1ac052d08c4beb511eb15396cd87b94a
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Mon Jul 17 15:38:01 2023 -0700

    remove fminmaxs since it's redundant with fminmax
Comment 37 Luke Kenneth Casson Leighton 2023-07-18 09:18:09 BST
(In reply to Jacob Lifshay from comment #36)
> I updated openpower-isa.git to use the new fminmax instruction, and removed
> fminmaxs while I was at it.

oh! err hang on - that needed some discussion rather than an
arbitrary decision.  looking at pages 734 onwards in Public v3.1
there are a considerable number of different precision min/max
operations.

>     remove fminmaxs since it's redundant with fminmax

this needs explanation and discussion rather than an unjustified
decision.  remember i have to explain it to the ISA WG and when
they ask the answer cannot be "i haven't the faintest idea why
jacob made that decision" :)
Comment 38 Jacob Lifshay 2023-07-18 09:40:52 BST
(In reply to Luke Kenneth Casson Leighton from comment #37)
> (In reply to Jacob Lifshay from comment #36)
> > I updated openpower-isa.git to use the new fminmax instruction, and removed
> > fminmaxs while I was at it.
> 
> oh! err hang on - that needed some discussion rather than an
> arbitrary decision.

i thought we already discussed it...

the *s ops are only necessary where they behave differently than the corresponding bfp64 ops, min/max do not behave differently therefore are redundant.

for a somewhat more detailed explanation see the programming note in the right column of page 522 (page 548) of powerisa v3.1b

> looking at pages 734 onwards in Public v3.1
> there are a considerable number of different precision min/max
> operations.

only double/quad precision -- those are not redundant because their input/outputs are encoded differently (bfp64 vs. bfp128). the corresponding single ops would use the bfp64 encoding so can be eliminated due to redundancy.
Comment 39 Jacob Lifshay 2023-07-19 04:12:40 BST
added fminmax tests for all the corner cases I could think of, found and fixed some pseudocode bugs. I'm now happy with submitting the fminmax pseudocode (everything else still needs more review though).

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=56e5a9b4562cebb88ceb6bb6512798327e57468d

commit 56e5a9b4562cebb88ceb6bb6512798327e57468d
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Tue Jul 18 20:01:30 2023 -0700

    add fminmax tests with corresponding pseudocode fixes


https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;h=6a352d8538954c57a5af7d6bdf703747abbbb776

commit 6a352d8538954c57a5af7d6bdf703747abbbb776
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Tue Jul 18 20:02:58 2023 -0700

    sync fminmax pseudocode fixes from openpower-isa.git
Comment 40 Jacob Lifshay 2023-07-19 04:15:36 BST
(In reply to Jacob Lifshay from comment #38)
> (In reply to Luke Kenneth Casson Leighton from comment #37)
> > (In reply to Jacob Lifshay from comment #36)
> > > I updated openpower-isa.git to use the new fminmax instruction, and removed
> > > fminmaxs while I was at it.
> > 
> > oh! err hang on - that needed some discussion rather than an
> > arbitrary decision.
> 
> i thought we already discussed it...

yes, we did discuss it, just neither of us read the previous comments before commenting again: https://bugs.libre-soc.org/show_bug.cgi?id=1057#c33
Comment 41 Luke Kenneth Casson Leighton 2023-12-03 02:33:49 GMT
(In reply to Jacob Lifshay from comment #40)
> just neither of us read the previous comments before
> commenting again: https://bugs.libre-soc.org/show_bug.cgi?id=1057#c33

facepalm moment :)

ok what's left on this one?

* TODO: should minmax use RA_OR_ZERO in the csv?
    it uses (RA|0) in the pseudo-code...

(answer: yes, always) DONE
https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=6626fc852

* WIP: add/modify unit tests for minmax
  * TODO: add more unit tests for minmax, testing for RA=0 and *w variants
    https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/test/bitmanip/av_cases.py;hb=HEAD

  move to new task
Comment 42 Jacob Lifshay 2023-12-03 02:40:48 GMT
(In reply to Luke Kenneth Casson Leighton from comment #41)
> ok what's left on this one?

looks like moving the additional tests to a new task leaves this one completed!
Comment 43 Luke Kenneth Casson Leighton 2023-12-03 05:00:27 GMT
(In reply to Jacob Lifshay from comment #42)

> looks like moving the additional tests to a new task leaves this one
> completed!

surpriiise :) RFP time...