> -----Original Message-----
> From: Richard Sandiford <[email protected]>
> Sent: Monday, October 9, 2023 10:56 AM
> To: Tamar Christina <[email protected]>
> Cc: Richard Biener <[email protected]>; [email protected];
> nd <[email protected]>; Richard Earnshaw <[email protected]>;
> Marcus Shawcroft <[email protected]>; Kyrylo Tkachov
> <[email protected]>
> Subject: Re: [PATCH]AArch64 Add SVE implementation for cond_copysign.
>
> Tamar Christina <[email protected]> writes:
> >> -----Original Message-----
> >> From: Richard Sandiford <[email protected]>
> >> Sent: Saturday, October 7, 2023 10:58 AM
> >> To: Richard Biener <[email protected]>
> >> Cc: Tamar Christina <[email protected]>;
> >> [email protected]; nd <[email protected]>; Richard Earnshaw
> >> <[email protected]>; Marcus Shawcroft
> >> <[email protected]>; Kyrylo Tkachov
> <[email protected]>
> >> Subject: Re: [PATCH]AArch64 Add SVE implementation for cond_copysign.
> >>
> >> Richard Biener <[email protected]> writes:
> >> > On Thu, Oct 5, 2023 at 10:46 PM Tamar Christina
> >> <[email protected]> wrote:
> >> >>
> >> >> > -----Original Message-----
> >> >> > From: Richard Sandiford <[email protected]>
> >> >> > Sent: Thursday, October 5, 2023 9:26 PM
> >> >> > To: Tamar Christina <[email protected]>
> >> >> > Cc: [email protected]; nd <[email protected]>; Richard Earnshaw
> >> >> > <[email protected]>; Marcus Shawcroft
> >> >> > <[email protected]>; Kyrylo Tkachov
> >> <[email protected]>
> >> >> > Subject: Re: [PATCH]AArch64 Add SVE implementation for
> >> cond_copysign.
> >> >> >
> >> >> > Tamar Christina <[email protected]> writes:
> >> >> > >> -----Original Message-----
> >> >> > >> From: Richard Sandiford <[email protected]>
> >> >> > >> Sent: Thursday, October 5, 2023 8:29 PM
> >> >> > >> To: Tamar Christina <[email protected]>
> >> >> > >> Cc: [email protected]; nd <[email protected]>; Richard
> >> >> > >> Earnshaw <[email protected]>; Marcus Shawcroft
> >> >> > >> <[email protected]>; Kyrylo Tkachov
> >> >> > <[email protected]>
> >> >> > >> Subject: Re: [PATCH]AArch64 Add SVE implementation for
> >> cond_copysign.
> >> >> > >>
> >> >> > >> Tamar Christina <[email protected]> writes:
> >> >> > >> > Hi All,
> >> >> > >> >
> >> >> > >> > This adds an implementation for masked copysign along with
> >> >> > >> > an optimized pattern for masked copysign (x, -1).
> >> >> > >>
> >> >> > >> It feels like we're ending up with a lot of AArch64-specific
> >> >> > >> code that just hard- codes the observation that changing the
> >> >> > >> sign is equivalent to changing the top bit. We then need to
> >> >> > >> make sure that we choose the best way of changing the top bit
> >> >> > >> for any
> >> given situation.
> >> >> > >>
> >> >> > >> Hard-coding the -1/negative case is one instance of that.
> >> >> > >> But it looks like we also fail to use the best sequence for SVE2.
> >> >> > >> E.g.
> >> >> > >> [https://godbolt.org/z/ajh3MM5jv]:
> >> >> > >>
> >> >> > >> #include <stdint.h>
> >> >> > >>
> >> >> > >> void f(double *restrict a, double *restrict b) {
> >> >> > >> for (int i = 0; i < 100; ++i)
> >> >> > >> a[i] = __builtin_copysign(a[i], b[i]); }
> >> >> > >>
> >> >> > >> void g(uint64_t *restrict a, uint64_t *restrict b, uint64_t c) {
> >> >> > >> for (int i = 0; i < 100; ++i)
> >> >> > >> a[i] = (a[i] & ~c) | (b[i] & c); }
> >> >> > >>
> >> >> > >> gives:
> >> >> > >>
> >> >> > >> f:
> >> >> > >> mov x2, 0
> >> >> > >> mov w3, 100
> >> >> > >> whilelo p7.d, wzr, w3
> >> >> > >> .L2:
> >> >> > >> ld1d z30.d, p7/z, [x0, x2, lsl 3]
> >> >> > >> ld1d z31.d, p7/z, [x1, x2, lsl 3]
> >> >> > >> and z30.d, z30.d, #0x7fffffffffffffff
> >> >> > >> and z31.d, z31.d, #0x8000000000000000
> >> >> > >> orr z31.d, z31.d, z30.d
> >> >> > >> st1d z31.d, p7, [x0, x2, lsl 3]
> >> >> > >> incd x2
> >> >> > >> whilelo p7.d, w2, w3
> >> >> > >> b.any .L2
> >> >> > >> ret
> >> >> > >> g:
> >> >> > >> mov x3, 0
> >> >> > >> mov w4, 100
> >> >> > >> mov z29.d, x2
> >> >> > >> whilelo p7.d, wzr, w4
> >> >> > >> .L6:
> >> >> > >> ld1d z30.d, p7/z, [x0, x3, lsl 3]
> >> >> > >> ld1d z31.d, p7/z, [x1, x3, lsl 3]
> >> >> > >> bsl z31.d, z31.d, z30.d, z29.d
> >> >> > >> st1d z31.d, p7, [x0, x3, lsl 3]
> >> >> > >> incd x3
> >> >> > >> whilelo p7.d, w3, w4
> >> >> > >> b.any .L6
> >> >> > >> ret
> >> >> > >>
> >> >> > >> I saw that you originally tried to do this in match.pd and
> >> >> > >> that the decision was to fold to copysign instead. But
> >> >> > >> perhaps there's a compromise where isel does something with
> >> >> > >> the (new) copysign canonical
> >> >> > form?
> >> >> > >> I.e. could we go with your new version of the match.pd patch,
> >> >> > >> and add some isel stuff as a follow-on?
>
> [A]
>
> >> >> > >>
> >> >> > >
> >> >> > > Sure if that's what's desired.... But..
> >> >> > >
> >> >> > > The example you posted above is for instance worse for x86
> >> >> > > https://godbolt.org/z/x9ccqxW6T where the first operation has
> >> >> > > a dependency chain of 2 and the latter of 3. It's likely any
> >> >> > > open coding of this
> >> >> > operation is going to hurt a target.
> >> >> > >
> >> >> > > So I'm unsure what isel transform this into...
> >> >> >
> >> >> > I didn't mean that we should go straight to using isel for the
> >> >> > general case, just for the new case. The example above was
> >> >> > instead trying to show the general point that hiding the logic
> >> >> > ops in target code is
> >> a double-edged sword.
> >> >>
> >> >> I see.. but the problem here is that transforming copysign (x, -1)
> >> >> into (x | 0x8000000) would require an integer operation on an FP
> >> >> value. I'm happy to do it but it seems like it'll be an AArch64
> >> >> only thing
> >> anyway.
> >> >>
> >> >> If we want to do this we need to check can_change_mode_class or a
> hook.
> >> >> Most targets including x86 reject the conversion. So it'll just
> >> >> be effectively an AArch64 thing.
> >> >>
> >> >> You're right that the actual equivalent transformation is this
> >> >> https://godbolt.org/z/KesfrMv5z But the target won't allow it.
> >> >>
> >> >> >
> >> >> > The x86_64 example for the -1 case would be
> >> >> > https://godbolt.org/z/b9s6MaKs8 where the isel change would be
> >> >> > an improvement. Without that, I guess
> >> >> > x86_64 will need to have a similar patch to the AArch64 one.
> >> >> >
> >> >>
> >> >> I think that's to be expected. I think it's logical that every
> >> >> target just needs to implement their optabs optimally.
> >> >>
> >> >> > That said, https://godbolt.org/z/e6nqoqbMh suggests that
> >> >> > powerpc64 is probably relying on the current copysign -> neg/abs
> transform.
> >> >> > (Not sure why the second function uses different IVs from the
> >> >> > first.)
> >> >> >
> >> >> > Personally, I wouldn't be against a target hook that indicated
> >> >> > whether float bit manipulation is "free" for a given mode, if it
> >> >> > comes to
> >> that.
> >> >>
> >> >> I'm really sure Richi would agree there. Generally speaking I
> >> >> don't think people see doing FP operations on Int as beneficial.
> >> >> But it's always
> >> the case on AArch64.
> >> >
> >> > IIRC we're doing fpclassify "expansion" early for example.
> >> >
> >> > Note the issue I see is that the middle-end shouldn't get in the
> >> > way of targets that have a copysign optab. In case it's worthwhile
> >> > to special-case a "setsign" thing then maybe provide an optab for
> >> > that as well. Now, that doesn't help if we want setsign to be
> >> > expanded from the middle-end but still wan the copysign optab (and
> >> > not require targets to implement both if they want to escape
> >> > middle-end generic
> >> expansion of setsign).
> >> >
> >> > But yes, I also thought the , 1 and , -1 cases could be special
> >> > cased by RTL expansion (or ISEL). But it would need to invoke
> >> > costing which likely means target specific changes anyway... :/
> >>
> >> FWIW, if we had the target hook I suggested, I don't think AArch64
> >> would need to implement copysign or xorsign optabs. That's probably
> >> true of
> >> AArch32 too (at least for all combinations that are likely to matter these
> days).
> >>
> >> I'd go one step further and say that, if a target wants to do its own
> >> thing for copysign and xorsign, it clearly doesn't meet the
> >> requirement that bit manipulation of floats is "free" for that mode.
> >>
> >
> > So I'm aware I have no say here, but I'll make one last effort.
> >
> > The patch isn't just implementing the fneg (fabs ()) optimization just
> because.
> > The location where it's implemented makes a big difference.
> >
> > If this optimization is done late, it doesn't fix the regression
> > fully, because doing It after all optimization passes have run means it
> > can't
> properly be optimized.
> >
> > The point of doing the rewrite early to ORR accomplished two things:
> >
> > 1. It makes PRE realize that the block it's splitting would only have 1
> instruction in it
> > and that such a split is not beneficial. This is why I'm against doing
> > such
> optimizations
> > so later. Optimizations don’t' happen in isolation, and when they make
> sense should
> > happen early. Transforming fneg (fabs (..)) in isel not only feels
> > wrong but
> results in a 4%
> > performance loss.
> >
> > 2. Doing it early also gets the ORRs to be reassociated changing where the
> loop dependent
> > variable lands. Early makes it land in the merging MOVPRFX rather than
> requiring a SEL
> > at the end of the iteration.
> >
> > Replacing the fneg (fabs (..)) with copysign accomplishes 1 but no 2.
> > This results in a 2-3% loss, but I can live with that given doing 1 gets us
> > back
> to GCC 12 levels.
> >
> > Doing fneg (fabs (..)) in isel would have no meaning for me and not
> > fix the regression. I won't be looking to do that in that case.
> >
> > If it's acceptable I can transform COPYSIGN (X, -1) in gimple-isel.
>
> FWIW, this is what I meant by [A] above. I.e. go with your latest target-
> independent change, and make isel replace COPYSIGN (X, -1) with logic
> operations, under control of a new target hook that says that logic operations
> on float values are as cheap as logic operations on integer values (for a
> given
> float mode). In future, hopefully all COPYSIGNs and XORSIGNs would be
> handled the same way, under control of the same hook, but that would be a
> separate future change.
Ok, would you prefer isel or in the expander that Richi suggested?
I assume isel is better if the intention is to later remove the expansion code?
> I wasn't suggesting recognising (fneg (fabs )) in isel.
>
Ok, I wasn't sure as there were many interleaving threads.
> But like I say, beware of powerpc64. It seems to rely on the current reverse
> transformation of (copysign x -1) to (fneg (fabs x)).
> (Could be wrong, but would be worth checking.)
I'll do a check, thanks!
Tamar
> Thanks,
> Richard
>
>
>
> > So before I start on this, Would this be acceptable for you?
> >
> > Thanks,
> > Tamar
> >
> >> > So I have no good advice here but I hoped that even the generic
> >> > target specific copysign implementation with and & xor would
> >> > eventually be optimized later on RTL for constant second arg?
> >>
> >> Yeah. It looks like the required logic is there for scalars, it just
> >> needs extending to vectors.
> >>
> >> The patch below (untested beyond simple cases) seems to be enough to
> >> fix it, but using the simplify routines even for CONST_INTs might be
> controversial.
> >>
> >> Thanks,
> >> Richard
> >>
> >>
> >> diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc index
> >> bd9443dbcc2..5a9b1745673 100644
> >> --- a/gcc/simplify-rtx.cc
> >> +++ b/gcc/simplify-rtx.cc
> >> @@ -3409,20 +3409,20 @@
> simplify_context::simplify_binary_operation_1
> >> (rtx_code code,
> >>
> >> /* Canonicalize (X & C1) | C2. */
> >> if (GET_CODE (op0) == AND
> >> - && CONST_INT_P (trueop1)
> >> - && CONST_INT_P (XEXP (op0, 1)))
> >> + && CONSTANT_P (trueop1)
> >> + && CONSTANT_P (XEXP (op0, 1)))
> >> {
> >> - HOST_WIDE_INT mask = GET_MODE_MASK (mode);
> >> - HOST_WIDE_INT c1 = INTVAL (XEXP (op0, 1));
> >> - HOST_WIDE_INT c2 = INTVAL (trueop1);
> >> + rtx c1 = XEXP (op0, 1);
> >> + rtx c2 = trueop1;
> >>
> >> /* If (C1&C2) == C1, then (X&C1)|C2 becomes C2. */
> >> - if ((c1 & c2) == c1
> >> + if (rtx_equal_p (simplify_binary_operation (AND, mode, c1, c2),
> >> +c1)
> >> && !side_effects_p (XEXP (op0, 0)))
> >> return trueop1;
> >>
> >> /* If (C1|C2) == ~0 then (X&C1)|C2 becomes X|C2. */
> >> - if (((c1|c2) & mask) == mask)
> >> + if (rtx_equal_p (simplify_binary_operation (IOR, mode, c1, c2),
> >> + CONSTM1_RTX (mode)))
> >> return simplify_gen_binary (IOR, mode, XEXP (op0, 0), op1);
> >> }
> >>
> >> @@ -3732,7 +3732,7 @@ simplify_context::simplify_binary_operation_1
> >> (rtx_code code,
> >> machines, and also has shorter instruction path length. */
> >> if (GET_CODE (op0) == AND
> >> && GET_CODE (XEXP (op0, 0)) == XOR
> >> - && CONST_INT_P (XEXP (op0, 1))
> >> + && CONSTANT_P (XEXP (op0, 1))
> >> && rtx_equal_p (XEXP (XEXP (op0, 0), 0), trueop1))
> >> {
> >> rtx a = trueop1;
> >> @@ -3746,7 +3746,7 @@ simplify_context::simplify_binary_operation_1
> >> (rtx_code code,
> >> /* Similarly, (xor (and (xor A B) C) B) as (ior (and A C) (and B
> >> ~C)) */
> >> else if (GET_CODE (op0) == AND
> >> && GET_CODE (XEXP (op0, 0)) == XOR
> >> - && CONST_INT_P (XEXP (op0, 1))
> >> + && CONSTANT_P (XEXP (op0, 1))
> >> && rtx_equal_p (XEXP (XEXP (op0, 0), 1), trueop1))
> >> {
> >> rtx a = XEXP (XEXP (op0, 0), 0);
> >> --
> >> 2.25.1
> >>
