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 883173858D32 for ; Mon, 27 Feb 2023 14:51:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 883173858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=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 5E4FAC14; Mon, 27 Feb 2023 06:52:03 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A40F73F67D; Mon, 27 Feb 2023 06:51:19 -0800 (PST) From: Richard Sandiford To: Richard Biener Mail-Followup-To: Richard Biener ,Michael Collison , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: Michael Collison , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] vect: Check that vector factor is a compile-time constant References: <3356a2e0-b402-de07-9374-6e5b5c59a2f2@rivosinc.com> Date: Mon, 27 Feb 2023 14:51:18 +0000 In-Reply-To: (Richard Biener's message of "Wed, 22 Feb 2023 09:20:13 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-34.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: FWIW, this patch looks good to me. I'd argue it's a regression fix of kinds, in that the current code was correct before variable VF and became incorrect after variable VF. It might be possible to trigger the problem on SVE too, with a sufficiently convoluted test case. (Haven't tried though.) Richard Biener writes: > On Wed, Feb 22, 2023 at 12:03 AM Michael Collison wrote: >> >> While working on autovectorizing for the RISCV port I encountered an >> issue where vect_do_peeling assumes that the vectorization factor is a >> compile-time constant. The vectorization is not a compile-time constant >> on RISCV. >> >> Tested on RISCV and x86_64-linux-gnu. Okay? > > I wonder how you arrive at prologue peeling with a non-constant VF? Not sure about the RVV case, but I think it makes sense in principle. E.g. if some ISA takes the LOAD_LEN rather than fully-predicated approach, it can't easily use the first iteration of the vector loop to do peeling for alignment. (At least, the IV steps would then no longer match VF for all iterations.) I guess it could use a *different* vector loop, but we don't support that yet. There are also some corner cases for which we still don't support predicated loops and instead fall back on an unpredicated VLA loop followed by a scalar epilogue. Peeling for alignment would then require a scalar prologue too. > In any case it would probably be better to use constant_lower_bound (vf) > here? Also it looks wrong to apply this limit in case we are using > a fully masked main vector loop. But as said, the specific case of > non-constant VF and prologue peeling probably wasn't supposed to happen, > instead the prologue usually is applied via an offset to a fully masked loop? Hmm, yeah, agree constant_lower_bound should work too. Thanks, Richard > Richard? > > Thanks, > Richard. > >> Michael >> >> gcc/ >> >> * tree-vect-loop-manip.cc (vect_do_peeling): Verify >> that vectorization factor is a compile-time constant. >> >> --- >> gcc/tree-vect-loop-manip.cc | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc >> index 6aa3d2ed0bf..1ad1961c788 100644 >> --- a/gcc/tree-vect-loop-manip.cc >> +++ b/gcc/tree-vect-loop-manip.cc >> @@ -2930,7 +2930,7 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree >> niters, tree nitersm1, >> niters = vect_build_loop_niters (loop_vinfo, &new_var_p); >> /* It's guaranteed that vector loop bound before vectorization is at >> least VF, so set range information for newly generated var. */ >> - if (new_var_p) >> + if (new_var_p && vf.is_constant ()) >> { >> value_range vr (type, >> wi::to_wide (build_int_cst (type, vf)), >> -- >> 2.34.1 >>