On Tue, Aug 31, 2021 at 06:41:30PM -0500, Segher Boessenkool wrote:
> Hi!
>
> Please do two separate patches. The first that adds the instruction
> (with a bit pattern, i.e. integer, input), and perhaps a second pattern
> that has an fp as input and uses it if the constant is valid for the
> insn (survives being converted to SP and back to DP (or the other way
> around), and is not denormal). That can be two patches if you want,
> but :-)
Well we already have the two stages for the built-in. But the point of the
work is to make it work without the explicit builtin.
> Having the integer intermediate step not only makes the code hugely less
> complicated, but is also allows e.g.
> ===
> typedef unsigned long long v2u64 __attribute__ ((vector_size (16)));
> v2u64 f(void)
> {
> v2u64 x = { 0x8000000000000000, 0x8000000000000000 };
> return x;
> }
> ===
>
> to be optimised properly.
>
> The second part is letting the existing code use such FP (and integer!)
> contants.
It is somewhat harder to do this as integer values, because you have to check
whether all of the bits are correct, including skipping the SF denormal bits.
I tend to think in real life, the only win will be be -0.0
(i.e. 0x8000000000000000 that you mentioned). I doubt many V2DI constants will
be appropriate canidates for this.
>
> On Wed, Aug 25, 2021 at 03:46:43PM -0400, Michael Meissner wrote:
> > +;; SF/DF/V2DF scalar or vector constant that can be loaded with XXSPLTIDP
> > +(define_constraint "eF"
> > + "A vector constant that can be loaded with the XXSPLTIDP instruction."
> > + (match_operand 0 "xxspltidp_operand"))
>
> vector *or float*. It should allow all vectors, not just FP ones.
>
> > +;; Return 1 if operand is a SF/DF CONST_DOUBLE or V2DF CONST_VECTOR that
> > can be
> > +;; loaded via the ISA 3.1 XXSPLTIDP instruction.
> > +(define_predicate "xxspltidp_operand"
> > + (match_code "const_double,const_vector,vec_duplicate")
> > +{
> > + HOST_WIDE_INT value = 0;
> > + return xxspltidp_constant_p (op, mode, &value);
> > +})
>
> Don't do that. Factor the code properly. A predicate function should
> never have side effects.
>
> Since this is the only place you want to convert the value to its bit
> pattern, you should just do that here.
>
> (Btw, initialising the value (although the function always writes it) is
> not defensive programming, it is hiding bugs. IMNSHO :-) )
And avoiding warnings.
>
> > +bool
> > +xxspltidp_constant_p (rtx op,
> > + machine_mode mode,
> > + HOST_WIDE_INT *constant_ptr)
> > +{
> > + *constant_ptr = 0;
>
> And a second time, too! Don't do either.
>
> > + if (!TARGET_XXSPLTIDP || !TARGET_PREFIXED || !TARGET_VSX)
> > + return false;
>
> This is the wrong place to test these. It belongs in the caller.
>
> > + if (CONST_VECTOR_P (op))
> > + {
> > + element = CONST_VECTOR_ELT (op, 0);
> > + if (!rtx_equal_p (element, CONST_VECTOR_ELT (op, 1)))
> > + return false;
> > + }
>
> const_vec_duplicate_p
Ok.
> (But you actually should check if the bit pattern is valid, nothing
> more, nothing less).
>
> > + /* Don't return true for 0.0 since that is easy to create without
> > + XXSPLTIDP. */
> > + if (element == CONST0_RTX (mode))
> > + return false;
>
> Don't do that. Instead have whatever decides what insn to use choose
> more directly.
>
> > +/* Whether a permute type instruction is a prefixed instruction. This is
> > + called from the prefixed attribute processing. */
> > +
> > +bool
> > +prefixed_permute_p (rtx_insn *insn)
>
> What does this have to do with this patch?
This is used by the prefixed type attribute to know that the xxsplti{w,dp,32dx}
instructions are prefixed by default, but unlike paddi, they don't want a
leading 'p' in the instruction.
The alternative is for each of the moves to add an explicit prefixed attribute,
instead of letting the default be set.
> > +{
> > + rtx set = single_set (insn);
> > + if (!set)
> > + return false;
> > +
> > + rtx dest = SET_DEST (set);
> > + rtx src = SET_SRC (set);
> > + machine_mode mode = GET_MODE (dest);
> > +
> > + if (!REG_P (dest) && !SUBREG_P (dest))
> > + return false;
> > +
> > + switch (mode)
> > + {
> > + case DFmode:
> > + case SFmode:
> > + case V2DFmode:
> > + return xxspltidp_operand (src, mode);
>
> ??!!??
>
> That is not a permute insn at all.
>
> Perhaps you mean it is executed in the PM pipe on current
> implementations (all one of-em). That does not make it a permute insn.
> It is not a good idea to call insns that do not have semantics similar
> to permutations "permute".
I'll come up with a different name. But because it includes SPLAT-ing to the
other double word, I would tend to believe these instructions will always be a
permute class instruction.
>
> > @@ -7755,15 +7760,16 @@ (define_insn "movsf_hardfloat"
> > @@ -8051,20 +8057,21 @@ (define_insn "*mov<mode>_hardfloat32"
> > @@ -8091,19 +8098,19 @@ (define_insn "*mov<mode>_softfloat32"
> > @@ -8125,18 +8132,19 @@ (define_insn "*mov<mode>_hardfloat64"
> > @@ -8170,6 +8178,7 @@ (define_insn "*mov<mode>_softfloat64"
>
> It would be a good idea to merge many of these patterns again. We can
> do this now that we have the "isa" and "enabled" attributes.
I don't plan to do this right now.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
