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 :-)
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.
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 :-) )
> +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
(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?
> +{
> + 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".
> @@ -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.
Segher
