From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 969D43840C1B for ; Fri, 29 May 2020 16:47:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 969D43840C1B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 04TGl7v5019494; Fri, 29 May 2020 11:47:07 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 04TGl6jC019493; Fri, 29 May 2020 11:47:06 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 29 May 2020 11:47:06 -0500 From: Segher Boessenkool To: Richard Biener Cc: Martin =?utf-8?B?TGnFoWth?= , GCC Patches , Richard Sandiford Subject: Re: [stage1][PATCH] Lower VEC_COND_EXPR into internal functions. Message-ID: <20200529164706.GX31009@gate.crashing.org> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-17.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR 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 16:47:11 -0000 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. If you want to change the documented semantics of widely used functions, please propose that? Segher