RE: [PATCH 5/6 ver 3] rs6000, Add vector splat builtin support
On Wed, 2020-07-01 at 12:00 -0500, Segher Boessenkool wrote:
> Hi!
>
> On Mon, Jun 29, 2020 at 03:31:48PM -0700, Carl Love wrote:
> > On Mon, 2020-06-29 at 16:58 -0500, Segher Boessenkool wrote:
> > > On Mon, Jun 29, 2020 at 02:29:54PM -0700, Carl Love wrote:
> > > > Segher:
> > > >
> > > > On Thu, 2020-06-25 at 17:39 -0500, Segher Boessenkool wrote:
> > > > > > +;; Return 1 if op is a constant 32-bit floating point
> > > > > > value
> > > > > > +(define_predicate "f32bit_const_operand"
> > > > > > + (match_code "const_double")
> > > > > > +{
> > > > > > + if (GET_MODE (op) == SFmode)
> > > > > > +return 1;
> > > > > > +
> > > > > > + else if ((GET_MODE (op) == DFmode) && ((UINTVAL (op) >>
> > > > > > 32)
> > > > > > ==
> > > > > > 0))
> > > > > > + {
> > > > > > +/* Value fits in 32-bits */
> > > > > > +return 1;
> > > > > > +}
> > > > > > + else
> > > > > > +/* Not the expected mode. */
> > > > > > +return 0;
> > > > > > +})
> > > > >
> > > > > I don't think this is the correct test. What you want to see
> > > > > is
> > > > > if
> > > > > the
> > > > > number in "op" can be converted to an IEEE single-precision
> > > > > number,
> > > > > and
> > > > > back again, losslessly. (And subnormal SP numbers aren't
> > > > > allowed
> > > > > either, but NaNs and infinities are).
> > > >
> > > > The predicate is used with the xxsplitw_v4sf
> > > > define_expand. The
> > > > "user"
> > > > claims the given immediate bit pattern is the bit pattern for a
> > > > single
> > > > precision floating point number. The immediate value is not
> > > > converted
> > > > to a float. Rather we are splatting a bit pattern that the
> > > > "user"
> > > > already claims represents a 32-bit floating point value. I
> > > > just
> > > > need
> > > > to make sure the immediate value actually fits into 32-bits.
> > > >
> > > > I don't see that I need to check that the value can be
> > > > converted to
> > > > IEEE float and back.
> > >
> > > Ah, okay. Can you please put that in the function comment
> > > then? Right
> > > now it says
> > > ;; Return 1 if op is a constant 32-bit floating point value
> > > and that is quite misleading.
> >
> > Would the following be more clear
> >
> > ;; Return 1 if op is a constant bit pattern representing a floating
> > ;; point value that fits in 32-bits.
>
> Yes... But I still don't get it.
>
> Say you have the number 785.066650390625 . As a single-precision
> IEEE
> float, that is x_. But as a double-precision IEEE float, it
> is
> 0x4088__8000_, which does not fit in 32 bits!
The value 0x_ would be mode SFmode and the test would return 1.
We would be able to splat that 32-bit wide pattern.
The value 0x4088__8000_ would be DFmode but when you shift it
right 32 bits it would be non-zero and
else if ((GET_MODE (op) == DFmode) && ((UINTVAL (op) >> 32)
would be true and the function would return 1 incorrectly. Argh!
So yes, we have a problem. The second part is supposed to be checking
for all zero not non zero. The test should be:
else if ((GET_MODE (op) == DFmode) && (((UINTVAL (op) >> 32) == 0)
Carl
Re: [PATCH 5/6 ver 3] rs6000, Add vector splat builtin support
Hi!
On Mon, Jun 29, 2020 at 03:31:48PM -0700, Carl Love wrote:
> On Mon, 2020-06-29 at 16:58 -0500, Segher Boessenkool wrote:
> > On Mon, Jun 29, 2020 at 02:29:54PM -0700, Carl Love wrote:
> > > Segher:
> > >
> > > On Thu, 2020-06-25 at 17:39 -0500, Segher Boessenkool wrote:
> > > > > +;; Return 1 if op is a constant 32-bit floating point value
> > > > > +(define_predicate "f32bit_const_operand"
> > > > > + (match_code "const_double")
> > > > > +{
> > > > > + if (GET_MODE (op) == SFmode)
> > > > > +return 1;
> > > > > +
> > > > > + else if ((GET_MODE (op) == DFmode) && ((UINTVAL (op) >> 32)
> > > > > ==
> > > > > 0))
> > > > > + {
> > > > > +/* Value fits in 32-bits */
> > > > > +return 1;
> > > > > +}
> > > > > + else
> > > > > +/* Not the expected mode. */
> > > > > +return 0;
> > > > > +})
> > > >
> > > > I don't think this is the correct test. What you want to see is
> > > > if
> > > > the
> > > > number in "op" can be converted to an IEEE single-precision
> > > > number,
> > > > and
> > > > back again, losslessly. (And subnormal SP numbers aren't allowed
> > > > either, but NaNs and infinities are).
> > >
> > > The predicate is used with the xxsplitw_v4sf define_expand. The
> > > "user"
> > > claims the given immediate bit pattern is the bit pattern for a
> > > single
> > > precision floating point number. The immediate value is not
> > > converted
> > > to a float. Rather we are splatting a bit pattern that the "user"
> > > already claims represents a 32-bit floating point value. I just
> > > need
> > > to make sure the immediate value actually fits into 32-bits.
> > >
> > > I don't see that I need to check that the value can be converted to
> > > IEEE float and back.
> >
> > Ah, okay. Can you please put that in the function comment
> > then? Right
> > now it says
> > ;; Return 1 if op is a constant 32-bit floating point value
> > and that is quite misleading.
>
> Would the following be more clear
>
> ;; Return 1 if op is a constant bit pattern representing a floating
> ;; point value that fits in 32-bits.
Yes... But I still don't get it.
Say you have the number 785.066650390625 . As a single-precision IEEE
float, that is x_. But as a double-precision IEEE float, it is
0x4088__8000_, which does not fit in 32 bits!
Seghr
RE: [PATCH 5/6 ver 3] rs6000, Add vector splat builtin support
On Mon, 2020-06-29 at 16:58 -0500, Segher Boessenkool wrote:
> On Mon, Jun 29, 2020 at 02:29:54PM -0700, Carl Love wrote:
> > Segher:
> >
> > On Thu, 2020-06-25 at 17:39 -0500, Segher Boessenkool wrote:
> > > > +;; Return 1 if op is a constant 32-bit floating point value
> > > > +(define_predicate "f32bit_const_operand"
> > > > + (match_code "const_double")
> > > > +{
> > > > + if (GET_MODE (op) == SFmode)
> > > > +return 1;
> > > > +
> > > > + else if ((GET_MODE (op) == DFmode) && ((UINTVAL (op) >> 32)
> > > > ==
> > > > 0))
> > > > + {
> > > > +/* Value fits in 32-bits */
> > > > +return 1;
> > > > +}
> > > > + else
> > > > +/* Not the expected mode. */
> > > > +return 0;
> > > > +})
> > >
> > > I don't think this is the correct test. What you want to see is
> > > if
> > > the
> > > number in "op" can be converted to an IEEE single-precision
> > > number,
> > > and
> > > back again, losslessly. (And subnormal SP numbers aren't allowed
> > > either, but NaNs and infinities are).
> >
> > The predicate is used with the xxsplitw_v4sf define_expand. The
> > "user"
> > claims the given immediate bit pattern is the bit pattern for a
> > single
> > precision floating point number. The immediate value is not
> > converted
> > to a float. Rather we are splatting a bit pattern that the "user"
> > already claims represents a 32-bit floating point value. I just
> > need
> > to make sure the immediate value actually fits into 32-bits.
> >
> > I don't see that I need to check that the value can be converted to
> > IEEE float and back.
>
> Ah, okay. Can you please put that in the function comment
> then? Right
> now it says
> ;; Return 1 if op is a constant 32-bit floating point value
> and that is quite misleading.
Would the following be more clear
;; Return 1 if op is a constant bit pattern representing a floating
;; point value that fits in 32-bits.
Carl
Re: [PATCH 5/6 ver 3] rs6000, Add vector splat builtin support
On Mon, Jun 29, 2020 at 02:29:54PM -0700, Carl Love wrote:
> Segher:
>
> On Thu, 2020-06-25 at 17:39 -0500, Segher Boessenkool wrote:
> > > +;; Return 1 if op is a constant 32-bit floating point value
> > > +(define_predicate "f32bit_const_operand"
> > > + (match_code "const_double")
> > > +{
> > > + if (GET_MODE (op) == SFmode)
> > > +return 1;
> > > +
> > > + else if ((GET_MODE (op) == DFmode) && ((UINTVAL (op) >> 32) ==
> > > 0))
> > > + {
> > > +/* Value fits in 32-bits */
> > > +return 1;
> > > +}
> > > + else
> > > +/* Not the expected mode. */
> > > +return 0;
> > > +})
> >
> > I don't think this is the correct test. What you want to see is if
> > the
> > number in "op" can be converted to an IEEE single-precision number,
> > and
> > back again, losslessly. (And subnormal SP numbers aren't allowed
> > either, but NaNs and infinities are).
>
> The predicate is used with the xxsplitw_v4sf define_expand. The "user"
> claims the given immediate bit pattern is the bit pattern for a single
> precision floating point number. The immediate value is not converted
> to a float. Rather we are splatting a bit pattern that the "user"
> already claims represents a 32-bit floating point value. I just need
> to make sure the immediate value actually fits into 32-bits.
>
> I don't see that I need to check that the value can be converted to
> IEEE float and back.
Ah, okay. Can you please put that in the function comment then? Right
now it says
;; Return 1 if op is a constant 32-bit floating point value
and that is quite misleading.
Segher
RE: [PATCH 5/6 ver 3] rs6000, Add vector splat builtin support
Segher:
On Thu, 2020-06-25 at 17:39 -0500, Segher Boessenkool wrote:
> > +;; Return 1 if op is a constant 32-bit floating point value
> > +(define_predicate "f32bit_const_operand"
> > + (match_code "const_double")
> > +{
> > + if (GET_MODE (op) == SFmode)
> > +return 1;
> > +
> > + else if ((GET_MODE (op) == DFmode) && ((UINTVAL (op) >> 32) ==
> > 0))
> > + {
> > +/* Value fits in 32-bits */
> > +return 1;
> > +}
> > + else
> > +/* Not the expected mode. */
> > +return 0;
> > +})
>
> I don't think this is the correct test. What you want to see is if
> the
> number in "op" can be converted to an IEEE single-precision number,
> and
> back again, losslessly. (And subnormal SP numbers aren't allowed
> either, but NaNs and infinities are).
The predicate is used with the xxsplitw_v4sf define_expand. The "user"
claims the given immediate bit pattern is the bit pattern for a single
precision floating point number. The immediate value is not converted
to a float. Rather we are splatting a bit pattern that the "user"
already claims represents a 32-bit floating point value. I just need
to make sure the immediate value actually fits into 32-bits.
I don't see that I need to check that the value can be converted to
IEEE float and back.
Carl
Re: [PATCH 5/6 ver 3] rs6000, Add vector splat builtin support
Hi!
On Thu, Jun 18, 2020 at 03:20:18PM -0700, Carl Love wrote:
> * config/rs6000/altivec.md (UNSPEC_XXSPLTIW, UNSPEC_XXSPLTID,
> UNSPEC_XXSPLTI32DX): New.
> (vxxspltiw_v4si, vxxspltiw_v4sf_inst, vxxspltidp_v2df_inst,
> vxxsplti32dx_v4si_inst, vxxsplti32dx_v4sf_inst): New define_insn.
> (vxxspltiw_v4sf, vxxspltidp_v2df, vxxsplti32dx_v4si,
> vxxsplti32dx_v4sf.): New define_expands.
A whole bunch of these could be done with iterators (but that is then a
future improvement, no need to do it now).
> +(define_expand "xxsplti32dx_v4si"
> + [(set (match_operand:V4SI 0 "register_operand" "=wa")
> + (unspec:V4SI [(match_operand:V4SI 1 "register_operand" "wa")
> + (match_operand:QI 2 "u1bit_cint_operand" "n")
> + (match_operand:SI 3 "s32bit_cint_operand" "n")]
> + UNSPEC_XXSPLTI32DX))]
> + "TARGET_FUTURE"
> +{
> + int index = INTVAL (operands[2]);
> +
> + if (!BYTES_BIG_ENDIAN)
> +index = 1 - index;
> +
> + /* Instruction uses destination as a source. Do not overwrite source. */
> + emit_move_insn (operands[0], operands[1]);
> +
> + emit_insn (gen_xxsplti32dx_v4si_inst (operands[0], GEN_INT (index),
> + operands[3]));
> + DONE;
> +}
> + [(set_attr "type" "vecsimple")])
> +
> +(define_insn "xxsplti32dx_v4si_inst"
> + [(set (match_operand:V4SI 0 "register_operand" "+wa")
> + (unspec:V4SI [(match_operand:QI 1 "u1bit_cint_operand" "n")
> + (match_operand:SI 2 "s32bit_cint_operand" "n")]
> + UNSPEC_XXSPLTI32DX))]
> + "TARGET_FUTURE"
> + "xxsplti32dx %x0,%1,%2"
> + [(set_attr "type" "vecsimple")])
Hrm. Can we not just use two operands for that, the second with a "0"
constraint? Then register allocation will sort it all out (compare to
other instructions with this problem, rl[wd]imi is classical).
> +;; Return 1 if op is a 32-bit constant signed integer
> +(define_predicate "s32bit_cint_operand"
> + (and (match_code "const_int")
> + (match_test "(unsigned HOST_WIDE_INT)
> +(0x8000 + UINTVAL (op)) >> 32 == 0")))
You don't need the cast here: UINTVAL returns that type already, and
that cascades to everything else in the expression.
> +;; Return 1 if op is a constant 32-bit floating point value
> +(define_predicate "f32bit_const_operand"
> + (match_code "const_double")
> +{
> + if (GET_MODE (op) == SFmode)
> +return 1;
> +
> + else if ((GET_MODE (op) == DFmode) && ((UINTVAL (op) >> 32) == 0))
> + {
> +/* Value fits in 32-bits */
> +return 1;
> +}
> + else
> +/* Not the expected mode. */
> +return 0;
> +})
I don't think this is the correct test. What you want to see is if the
number in "op" can be converted to an IEEE single-precision number, and
back again, losslessly. (And subnormal SP numbers aren't allowed
either, but NaNs and infinities are).
Rest looks fine. Sorry it took me so long to spot this.
Segher
