From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 74019394F818 for ; Tue, 12 Jul 2022 12:32:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 74019394F818 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 42DA420043; Tue, 12 Jul 2022 12:32:06 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 3A0AA2C141; Tue, 12 Jul 2022 12:32:06 +0000 (UTC) Date: Tue, 12 Jul 2022 12:32:06 +0000 (UTC) From: Richard Biener To: Joel Hutton cc: Richard Sandiford , "gcc-patches@gcc.gnu.org" , Andre Simoes Dias Vieira Subject: RE: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Tue, 12 Jul 2022 12:32:08 -0000 On Thu, 30 Jun 2022, Joel Hutton wrote: > > We can go with a private vect_gimple_build function until we sort out the API > > issue to unblock Tamar (I'll reply to Richards reply with further thoughts on > > this) > > > > Done. > > > > Similarly are you ok with the use of gimple_extract_op? I would lean > > towards using it as it is cleaner, but I don't have strong feelings. > > > > I don't like using gimple_extract_op here, I think I outlined a variant that is > > even shorter. > > > > Done. > > Updated patches attached, bootstrapped and regression tested on aarch64. > > Tomorrow is my last working day at Arm, so it will likely be Andre that commits this/addresses any further comments. First sorry for the (repeated) delays. In the first patch I still see ECF_WIDEN, I don't like that, we use things like associative_binary_fn_p so for widening internal functions similar predicates should be used. In the second patch you add vec_widen_{add,sub} optabs +OPTAB_CD(vec_widen_add_optab, "add$a$b3") +OPTAB_CD(vec_widen_sub_optab, "sub$a$b3") but a) the names are that of regular adds which is at least confusing (if not wrong), b) there's no documentation for them in md.texi which, c) doesn't explain why they are necessary when we have vec_widen_[su]{add,sub}l_optab + internal_fn ifn = as_internal_fn (code.safe_as_fn_code ()); asks for safe_as_internal_fn () (just complete the API, also with safe_as_builtin_fn) + internal_fn lo, hi; + lookup_multi_internal_fn (ifn, &lo, &hi); + *code1 = as_combined_fn (lo); + *code2 = as_combined_fn (hi); in fact this probably shows that the guarding condition should be if (code.is_internal_fn ()) instead of if (code.is_fn_code ()). + optab1 = lookup_multi_ifn_optab (lo, !TYPE_UNSIGNED (vectype)); + optab2 = lookup_multi_ifn_optab (hi, !TYPE_UNSIGNED (vectype)); this shows the two lookup_ APIs are inconsistent in having two vs. one output, please make them consistent. I'd say give lookup_multi_internal_fn a enum { LO, HI } argument, returning the result. Given VEC_WIDEN_MULT has HI, LO, EVEN and ODD variants that sounds more future proof. The internal_fn stuff could probably get a 2nd eye from Richard. In the third patch I see unrelated and wrong changes like /* Check that the DR_INITs are compile-time constants. */ - if (!tree_fits_shwi_p (DR_INIT (dra)) - || !tree_fits_shwi_p (DR_INIT (drb))) + if (TREE_CODE (DR_INIT (dra)) != INTEGER_CST + || TREE_CODE (DR_INIT (drb)) != INTEGER_CST) break; please strip the patch down to relevant changes. - tree op = gimple_op (assign, i + 1); + tree op; + if (is_gimple_assign (stmt)) + op = gimple_op (stmt, i + 1); + else + op = gimple_call_arg (stmt, i); somebody added gimple_arg which can be used here doing op = gimple_arg (stmt, i); + tree lhs = is_gimple_assign (stmt) ? gimple_assign_lhs (stmt): + gimple_call_lhs (stmt); tree lhs = gimple_get_lhs (stmt); /* Check for an integer operation with the right code. */ - gassign *assign = dyn_cast (stmt_info->stmt); - if (!assign) + gimple* stmt = stmt_info->stmt; + if (!(is_gimple_assign (stmt) || is_gimple_call (stmt))) return 0; - tree_code rhs_code = gimple_assign_rhs_code (assign); - if (rhs_code != code && rhs_code != widened_code) + code_helper rhs_code; + if (is_gimple_assign (stmt)) + rhs_code = gimple_assign_rhs_code (stmt); + else + rhs_code = gimple_call_combined_fn (stmt); + + if (rhs_code.safe_as_tree_code () != code + && rhs_code.get_rep () != widened_code.get_rep ()) return 0; that's probably better refactored as if (is_gimple_assign (stmt)) { if (code check) return 0; } else if (is_gimple_call (..)) { .. } else return 0; otherwise the last patch looks reasonable. Richard.