Richard Biener <rguent...@suse.de> writes:
> On Thu, 20 Jun 2024, Richard Sandiford wrote:
>
>> Richard Biener <rguent...@suse.de> writes:
>> > On Mon, 17 Jun 2024, Richard Sandiford wrote:
>> >
>> >> Richard Biener <rguent...@suse.de> writes:
>> >> > On Fri, 14 Jun 2024, Richard Biener wrote:
>> >> >
>> >> >> On Fri, 14 Jun 2024, Richard Sandiford wrote:
>> >> >>
>> >> >> > Richard Biener <rguent...@suse.de> writes:
>> >> >> > > On Fri, 14 Jun 2024, Richard Sandiford wrote:
>> >> >> > >
>> >> >> > >> Richard Biener <rguent...@suse.de> writes:
>> >> >> > >> > The following retires vcond{,u,eq} optabs by stopping to use
>> >> >> > >> > them
>> >> >> > >> > from the middle-end. Targets instead (should) implement
>> >> >> > >> > vcond_mask
>> >> >> > >> > and vec_cmp{,u,eq} optabs. The PR this change refers to lists
>> >> >> > >> > possibly affected targets - those implementing these patterns,
>> >> >> > >> > and in particular it lists mips, sparc and ia64 as targets that
>> >> >> > >> > most definitely will regress while others might simply remove
>> >> >> > >> > their vcond{,u,eq} patterns.
>> >> >> > >> >
>> >> >> > >> > I'd appreciate testing, I do not expect fallout for x86 or
>> >> >> > >> > arm/aarch64.
>> >> >> > >> > I know riscv doesn't implement any of the legacy optabs. But
>> >> >> > >> > less
>> >> >> > >> > maintained vector targets might need adjustments.
>> >> >> > >> >
>> >> >> > >> > I want to get rid of those optabs for GCC 15. If I don't hear
>> >> >> > >> > from
>> >> >> > >> > you I will assume your target is fine.
>> >> >> > >>
>> >> >> > >> Great! Thanks for doing this.
>> >> >> > >>
>> >> >> > >> Is there a plan for how we should handle vector comparisons that
>> >> >> > >> have to be done as the inverse of the negated condition? Should
>> >> >> > >> targets simply not provide vec_cmp for such conditions and leave
>> >> >> > >> the target-independent code to deal with the fallout? (For a
>> >> >> > >> standalone comparison, it would invert the result. For a
>> >> >> > >> VEC_COND_EXPR
>> >> >> > >> it would swap the true and false values.)
>> >> >> > >
>> >> >> > > I would expect that the ISEL pass which currently deals with
>> >> >> > > finding
>> >> >> > > valid combos of .VCMP{,U,EQ} and .VCOND_MASK deals with this.
>> >> >> > > So how do we deal with this right now? I expect RTL expansion will
>> >> >> > > do the inverse trick, no?
>> >> >> >
>> >> >> > I think in practice (at least for the targets I've worked on),
>> >> >> > the target's vec_cmp handles the inversion itself. Thus the
>> >> >> > main optimisation done by targets' vcond patterns is to avoid
>> >> >> > the inversion (and instead swap the true/false values) when the
>> >> >> > "opposite" comparison is the native one.
>> >> >>
>> >> >> I see. I suppose whether or not vec_cmp is handled is determined
>> >> >> by a FAIL so it's somewhat difficult to determine this at ISEL time.
>> >>
>> >> In principle we could say that the predicates should accept only the
>> >> conditions that can be done natively. Then target-independent code
>> >> can apply the usual approaches to generating other conditions
>> >> (which tend to be replicated across targets anyway).
>> >
>> > Ah yeah, I suppose that would work. So we'd update the docs
>> > to say predicates are required to reject not handled compares
>> > and otherwise the expander may not FAIL?
>> >
>> > I'll note that expand_vec_cmp_expr_p already looks at the insn
>> > predicates, so adjusting vector lowering (and vectorization) to
>> > emit only recognized compares (and requiring folding to keep it at that)
>> > should be possible.
>> >
>> > ISEL would then mainly need to learn the trick of swapping vector
>> > cond arms on inverted masks. OTOH folding should also do that.
>>
>> Yeah.
>>
>> > Or do you suggest to allow all compares on GIMPLE and only fixup
>> > during ISEL? How do we handle vector lowering then? Would it be
>> > enough to require "any" condition code and thus we expect targets
>> > to implement enough codes so all compares can be handled by
>> > swapping/inversion?
>>
>> I'm not sure TBH. I can see the argument that "canonicalising"
>> conditions for the target could be either vector lowering or ISEL.
>>
>> If a target can only do == or != natively, for instance (is any target
>> like that?), then I think it should be ok for the predicates to accept
>> only that condition. Then the opposite != or == could be done using
>> vector lowering/ISEL, but ordered comparisons would need to be lowered
>> as though vec_cmp wasn't implemented at all.
>>
>> Something similar probably applies to FP comparisons if the handling
>> of unordered comparisons is limited.
>>
>> And if we do that, it might be easier for vector lowering to handle
>> everything itself, rather than try to predict what ISEL is going to do.
>
> I agree that as we have to handle completely unsupported cases in
> vector lowering anyway it's reasonable to try to force only supported
> ops after that.
>
> Note that when targets stop to advertise not supported compares then
> the vectorizer likely needs adjustments as well. We can of course
> put some common logic into the middle-end like making the
> expand_vec_cmp_expr_p function take additional optional arguments
> to indicate whether swapping operands or inverting the condition
> code makes the comparison supported.
>
> Do you know any target off head that restricts vec_cmp support via
> predicates? I'm not exactly sure how do implement such predicate
> and having a patch for a target with some vector coverage would
> be nice for implementing this. I'd start with the vector lowering
> bits obviously, I suspect there's quite some patterns in match.pd
> that need guarding in case there's no existing target doing such
> predication.
I'm not aware of a target that already does it, since I imagine the
pessimisation would be unacceptable. But here's what it might look like
for AArch64 Advanced SIMD:
diff --git a/gcc/config/aarch64/aarch64-simd.md
b/gcc/config/aarch64/aarch64-simd.md
index 01b084d8ccb..a3ce504cddd 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -3844,9 +3844,12 @@ (define_expand "cbranch<mode>4"
;; Patterns comparing two vectors to produce a mask.
+(define_predicate "int_vec_cmp_operator"
+ (match_code "lt,ge,le,gt,ltu,geu,leu,gtu,eq"))
+
(define_expand "vec_cmp<mode><mode>"
[(set (match_operand:VSDQ_I_DI 0 "register_operand")
- (match_operator 1 "comparison_operator"
+ (match_operator 1 "int_vec_cmp_operator"
[(match_operand:VSDQ_I_DI 2 "register_operand")
(match_operand:VSDQ_I_DI 3 "nonmemory_operand")]))]
"TARGET_SIMD"
@@ -3924,9 +3927,12 @@ (define_expand "vec_cmp<mode><mode>"
DONE;
})
+(define_predicate "fp_vec_cmp_operator"
+ (match_code "lt,ge,le,gt,eq"))
+
(define_expand "vec_cmp<mode><v_int_equiv>"
[(set (match_operand:<V_INT_EQUIV> 0 "register_operand")
- (match_operator 1 "comparison_operator"
+ (match_operator 1 "fp_vec_cmp_operator"
[(match_operand:VDQF 2 "register_operand")
(match_operand:VDQF 3 "nonmemory_operand")]))]
"TARGET_SIMD"
The FP case is tricky to decide because we do implement UNORDERED,
but seem to do it in a way that could signal invalid for sNaNs.
For -fno-signaling-nans, the approach of doing two ==s and
combining them is target-independent.
Thanks,
Richard
