From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x541.google.com (mail-ed1-x541.google.com [IPv6:2a00:1450:4864:20::541]) by sourceware.org (Postfix) with ESMTPS id A7C24384A033 for ; Mon, 15 Jun 2020 11:59:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A7C24384A033 Received: by mail-ed1-x541.google.com with SMTP id m21so11280076eds.13 for ; Mon, 15 Jun 2020 04:59:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=93ONHMMJ8monqnHL7ic/WtN7vU7UrkxGvMEtlDaZcgg=; b=r/a6n+npC/Ohwnr9M/5rMZTaVy8tMuCKxWPbkmYMIFAi/NpZ9GRAjEzHM42A8aQtY+ kp6wc+ZBv1v7SPkQh0GcgG1z8Ciopik/eCTwlT28prOwFAWzcmGHhakJ9/qnx8pT0aqL 65iy5Nt5ca2zplV1N5Vo7HoWvKBLZsjWIx752iFTy0D9bYgVmI+lYkjM+YA8IJ15Stwq sg2UFrdvLgbh+kJTKTpfCU1aGxwAdfCnhK92MCek+9QYrgYeRA69sacaYkiWthQAMGBg C4uuZFvxOqAdAWN3RCIpR/OM8lQFHeaSe9gOCr+vwXXt78xjYYHenM/BjFc+zMue16Iy CVjA== X-Gm-Message-State: AOAM530V7UddoX6DLomVk7KF6hGfudNcOgACaj6OjkGgNZ+wJgn4DkCP 4wFtIiuy5XFHHXxD4XxiW2ZdlhOTwYRSA4b8XLQ= X-Google-Smtp-Source: ABdhPJxM22JopmfnBerOx+wIdLI/rcjIs+qX60Ldz6HlyJDx4CtxMYedsSMCyxC3GVQeVcJrBjydzyC4CORPx3QZ4q0= X-Received: by 2002:a05:6402:1247:: with SMTP id l7mr22342782edw.61.1592222391711; Mon, 15 Jun 2020 04:59:51 -0700 (PDT) MIME-Version: 1.0 References: <20200529153933.GW31009@gate.crashing.org> <20200529170936.GY31009@gate.crashing.org> <20200529173758.GA31009@gate.crashing.org> <20200530130801.GD31009@gate.crashing.org> <16e3957c-e390-5984-b14e-dd3c70c3bd1c@suse.cz> <20200603182734.GA31009@gate.crashing.org> <3932cb72-4529-6755-bb35-45e11f1c3382@suse.cz> <77faed30-2ed8-8319-f552-34cad171c927@suse.cz> In-Reply-To: From: Richard Biener Date: Mon, 15 Jun 2020 13:59:40 +0200 Message-ID: Subject: Re: [stage1][PATCH] Lower VEC_COND_EXPR into internal functions. To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: Segher Boessenkool , GCC Patches , Richard Sandiford , David Edelsohn Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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: Mon, 15 Jun 2020 11:59:55 -0000 On Mon, Jun 15, 2020 at 1:19 PM Martin Li=C5=A1ka wrote: > > On 6/15/20 9:14 AM, Richard Biener wrote: > > On Fri, Jun 12, 2020 at 3:24 PM Martin Li=C5=A1ka wrot= e: > >> > >> On 6/12/20 11:43 AM, Richard Biener wrote: > >>> So ... how far are you with enforcing a split VEC_COND_EXPR? > >>> Thus can we avoid the above completely (even as intermediate > >>> state)? > >> > >> Apparently, I'm quite close. Using the attached patch I see only 2 tes= tsuite > >> failures: > >> > >> FAIL: gcc.dg/tree-ssa/pr68714.c scan-tree-dump-times reassoc1 " <=3D "= 1 > >> FAIL: gcc.target/i386/pr78102.c scan-assembler-times pcmpeqq 3 > >> > >> The first one is about teaching reassoc about the SSA_NAMEs in VEC_CON= D_EXPR. I haven't > >> analyze the second failure. > >> > >> I'm also not sure about the gimlification change, I see a superfluous = assignments: > >> vec_cond_cmp.5 =3D _1 =3D=3D _2; > >> vec_cond_cmp.6 =3D vec_cond_cmp.5; > >> vec_cond_cmp.7 =3D vec_cond_cmp.6; > >> _3 =3D VEC_COND_EXPR ; > >> ? > >> > >> So with the suggested patch, the EH should be gone as you suggested. R= ight? > > > > Right, it should be on the comparison already from the start. > > > > @@ -14221,9 +14221,13 @@ gimplify_expr (tree *expr_p, gimple_seq > > *pre_p, gimple_seq *post_p, > > case VEC_COND_EXPR: > > { > > enum gimplify_status r0, r1, r2; > > - > > r0 =3D gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, > > post_p, is_gimple_condexpr, fb_rvalue)= ; > > + tree xop0 =3D TREE_OPERAND (*expr_p, 0); > > + tmp =3D create_tmp_var_raw (TREE_TYPE (xop0), "vec_cond_cmp= "); > > + gimple_add_tmp_var (tmp); > > + gimplify_assign (tmp, xop0, pre_p); > > + TREE_OPERAND (*expr_p, 0) =3D tmp; > > r1 =3D gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, > > post_p, is_gimple_val, fb_rvalue); > > > > all of VEC_COND_EXPR can now be a simple goto expr_3; > > Works for me, thanks! > > > > > diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c > > index 494c9e9c20b..090fb52a2f1 100644 > > --- a/gcc/tree-ssa-forwprop.c > > +++ b/gcc/tree-ssa-forwprop.c > > @@ -3136,6 +3136,10 @@ pass_forwprop::execute (function *fun) > > if (code =3D=3D COND_EXPR > > || code =3D=3D VEC_COND_EXPR) > > { > > + /* Do not propagate into VEC_COND_EXPRs. */ > > + if (code =3D=3D VEC_COND_EXPR) > > + break; > > + > > > > err - remove the || code =3D=3D VEC_COND_EXPR instead? > > Yep. > > > > > @@ -2221,24 +2226,12 @@ expand_vector_operations (void) > > { > > gimple_stmt_iterator gsi; > > basic_block bb; > > - bool cfg_changed =3D false; > > > > FOR_EACH_BB_FN (bb, cfun) > > - { > > - for (gsi =3D gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi= )) > > - { > > - expand_vector_operations_1 (&gsi); > > - /* ??? If we do not cleanup EH then we will ICE in > > - verification. But in reality we have created wrong-code > > - as we did not properly transition EH info and edges to > > - the piecewise computations. */ > > - if (maybe_clean_eh_stmt (gsi_stmt (gsi)) > > - && gimple_purge_dead_eh_edges (bb)) > > - cfg_changed =3D true; > > - } > > - } > > > > I'm not sure about this. Consider the C++ testcase where > > the ?: is replaced by a division. If veclower needs to replace > > that with four scalrar division statements then the above > > still applies - veclower does not correctly duplicate EH info > > and EH edges to the individual divisions (and we do not know > > which component might trap). > > > > So please leave the above in. You can try if using integer > > division makes it break and add such a testcase if there's > > no coverage for this in the testsuite. > > I'm leaving that above. Can you please explain how can a division test-ca= se > be created? typedef long v2di __attribute__((vector_size(16))); v2di foo (v2di a, v2di b) { try { v2di res =3D a / b; return res; } catch (...) { return (v2di){}; } } with -fnon-call-exceptions I see in t.ii.090t.ehdisp (correctly): ;; basic block 2, loop depth 0 ;; pred: ENTRY [LP 1] _6 =3D a_4(D) / b_5(D); ;; succ: 5 ;; 3 while after t.ii.226t.veclower we have ;; basic block 2, loop depth 0 ;; pred: ENTRY _13 =3D BIT_FIELD_REF ; _14 =3D BIT_FIELD_REF ; _15 =3D _13 / _14; _16 =3D BIT_FIELD_REF ; _17 =3D BIT_FIELD_REF ; _18 =3D _16 / _17; _6 =3D {_15, _18}; res_7 =3D _6; _8 =3D res_7; ;; succ: 3 and all EH is gone and we'd ICE if you remove the above hunk. Hopefully. We still generate wrong-code obviously as we'd need to duplicate the EH info on each component division (and split blocks and generate extra EH edges). That's a pre-existing bug of course. I just wanted to avoid to create a new instance just because of the early instruction selection for VEC_COND_EXPR. > > > > What's missing from the patch is adjusting > > verify_gimple_assign_ternary from > > > > if (((rhs_code =3D=3D VEC_COND_EXPR || rhs_code =3D=3D COND_EXPR) > > ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1)) > > || !is_gimple_val (rhs2) > > || !is_gimple_val (rhs3)) > > { > > error ("invalid operands in ternary operation"); > > return true; > > > > to the same with the rhs_code =3D=3D VEC_COND_EXPR case removed. > > Hmm. I'm not sure I've got this comment. Why do we want to change it > and is it done wright in the patch? Ah, I missed the hunk you added. But the check should be an inclusive one, not an exclusive one and earlier accepting a is_gimple_condexpr is superfluous when you later reject the tcc_comparison part. Just testing is_gimple_val is better. So yes, remove your tree-cfg.c hunk and just adjust the above test. > > > > You'll likely figure the vectorizer still creates some VEC_COND_EXPRs > > with embedded comparisons. > > I've fixed 2 failing test-cases I mentioned in the previous email. > > Martin > > > > > Thanks, > > Richard. > > > > > >> Martin >