Re: [PATCH], Add PowerPC ISA 3.0 IEEE 128-bit floating point round to odd built-in functions

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi!

On Mon, Oct 02, 2017 at 02:01:57PM -0400, Michael Meissner wrote:
> On Fri, Sep 29, 2017 at 12:10:07PM -0500, Segher Boessenkool wrote:
> > On Thu, Sep 28, 2017 at 06:34:23PM -0400, Michael Meissner wrote:
> > > --- gcc/config/rs6000/rs6000.md	(revision 253267)
> > > +++ gcc/config/rs6000/rs6000.md	(working copy)
> > > @@ -14505,7 +14505,9 @@ (define_insn_and_split "trunc<mode>sf2_h
> > >    "#"
> > >    "&& 1"
> > >    [(set (match_dup 2)
> > > -	(unspec:DF [(match_dup 1)] UNSPEC_ROUND_TO_ODD))
> > > +	(unspec:DF [(float_truncate:DF
> > > +		     (match_dup 1))]
> > > +		   UNSPEC_ROUND_TO_ODD))
> > >     (set (match_dup 0)
> > >  	(float_truncate:SF (match_dup 2)))]
> > >  {
> > 
> > I don't think this is correct.  It says to first truncate the f128 to DF,
> > and then round it to odd; I think you want to do the truncation with
> > round-to-odd rounding mode already.
> > 
> > > +(define_insn "mul<mode>3_odd"
> > > +  [(set (match_operand:IEEE128 0 "altivec_register_operand" "=v")
> > > +	(unspec:IEEE128
> > > +	 [(mult:IEEE128
> > > +	   (match_operand:IEEE128 1 "altivec_register_operand" "v")
> > > +	   (match_operand:IEEE128 2 "altivec_register_operand" "v"))]
> > > +	 UNSPEC_ROUND_TO_ODD))]
> > > +  "TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<MODE>mode)"
> > > +  "xsmulqpo %0,%1,%2"
> > > +  [(set_attr "type" "vecfloat")
> > > +   (set_attr "size" "128")])
> > 
> > Similar here (and everywhere else): it does an f128 mul, so rounding
> > with whatever rounding mode is current, and *then* it rounds to odd.

> > Everything else looks fine, but that unspec thing needs fixing.  Can be
> > later, things will likely work for now, so okay for trunk.  Thanks.
> > 
> > How do other ports deal with this?  Insns with a specific rounding mode?
> > Have a separate unspec for every operation?  Not very nice either :-(
> 
> I just want to do the minimal work so the glibc people can use this
> instruction, but I am not that motivated to go back and modify it further.  So,
> I would prefer to do it right before committing it, unless you want to refine
> it.
> 
> As I see it, there are 2 ways to encode the RTL:
> 
>     1)	Use one UNSPEC name, and add the operation inside of the spec;

But this is incorrect as far as I see: it does not correctly describe
what the instruction does.

>     2)	Add separate UNSPEC's for each operation.
> 
> Either one is fine with me.  If you would prefer separate names, I can do it.

Yes please.

Thanks,


Segher