From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 13FA2386F439 for ; Fri, 29 May 2020 17:05:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 13FA2386F439 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=richard.sandiford@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BB5551045; Fri, 29 May 2020 10:05:16 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F0DDF3F52E; Fri, 29 May 2020 10:05:15 -0700 (PDT) From: Richard Sandiford To: Segher Boessenkool Mail-Followup-To: Segher Boessenkool ,Richard Biener , Martin =?utf-8?Q?Li=C5=A1ka?= , GCC Patches , richard.sandiford@arm.com Cc: Richard Biener , Martin =?utf-8?Q?Li?= =?utf-8?Q?=C5=A1ka?= , GCC Patches Subject: Re: [stage1][PATCH] Lower VEC_COND_EXPR into internal functions. References: <20200521201654.GX31009@gate.crashing.org> <1125c547-9245-12de-6c49-fa31361b359c@suse.cz> <5F0F38D2-DC37-4B67-8F48-C4C2FCC7D4CC@gmail.com> <3f84124b-f77e-1f8e-68d1-f0a7892d07b0@suse.cz> <20200529164706.GX31009@gate.crashing.org> Date: Fri, 29 May 2020 18:05:14 +0100 In-Reply-To: <20200529164706.GX31009@gate.crashing.org> (Segher Boessenkool's message of "Fri, 29 May 2020 11:47:06 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-15.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 May 2020 17:05:18 -0000 Segher Boessenkool writes: > On Fri, May 29, 2020 at 02:43:12PM +0200, Richard Biener wrote: >> So I tried to understand the circumstances the rs6000 patterns FAIL >> but FAILed ;) It looks like some outs of rs6000_emit_vector_cond_expr >> are unwarranted and the following should work: >> >> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c >> index 8435bc15d72..5503215a00a 100644 >> --- a/gcc/config/rs6000/rs6000.c >> +++ b/gcc/config/rs6000/rs6000.c >> @@ -14638,8 +14638,7 @@ rs6000_emit_vector_compare (enum rtx_code rcode, > > (Different function, btw) > >> rtx mask2; >> >> rev_code = reverse_condition_maybe_unordered (rcode); >> - if (rev_code == UNKNOWN) >> - return NULL_RTX; >> + gcc_assert (rev_code != UNKNOWN); > > reverse_condition_maybe_unordered is documented as possibly returning > UNKNOWN. The current implementation doesn't, sure. But fix that first? > > rs6000_emit_vector_compare can fail for several other reasons, too -- > including when rs6000_emit_vector_compare_inner fails. > >> @@ -14737,8 +14736,7 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx >> op_true, rtx op_false, >> rtx cond2; >> bool invert_move = false; >> >> - if (VECTOR_UNIT_NONE_P (dest_mode)) >> - return 0; >> + gcc_assert (VECTOR_UNIT_NONE_P (dest_mode)); > > Why can this condition never be true? (Missing a ! btw) > > It needs a big comment if you want to make wide assumptions like that, > in any case. Pretty much *all* (non-trivial) asserts need an explanation. > > (And perhaps VECTOR_UNIT_ALTIVEC_OR_VSX_P is better). > >> /* Get the vector mask for the given relational operations. */ >> mask = rs6000_emit_vector_compare (rcode, cc_op0, cc_op1, mask_mode); >> >> if (!mask) >> return 0; >> >> fail but that function recurses heavily - from reading >> rs6000_emit_vector_compare_inner >> it looks like power can do a lot of compares but floating-point LT which >> reverse_condition_maybe_unordered would turn into UNGE which is not >> handled either. >> But then rs6000_emit_vector_compare just tries GT for that anyway (not UNGE) so >> it is actually be handled (but should not?). >> >> So I bet the expansion of the patterns cannot fail at the moment. Thus I'd >> replace the FAIL with a gcc_unreachable () and see if we have test >> coverage for those >> FAILs. > > I am not comfortable with that at all. > >> Segher - do you actually know this code to guess why the patterns are defensive? > > Yes. In that case, can you give a specific example in which the patterns do actually fail? I think Richard's point is that even the current compiler will ICE if the vcond* patterns fail. All Martin's patch did was expose that via the extra static checking we get for directly-mapped internal fns. If you want us to fix that by providing a fallback, we need to know what the fallback should do. E.g. the obvious thing would be to emit the embedded comparison separately and then emit bitwise operations to implement the select. But in the powerpc case, it's actually the comparison that's the potential problem, so that expansion would just kick the can further down the road. So which vector comparisons doesn't powerpc support, and what should the fallback vcond* expansion for them be? Richard