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 CA5333858416 for ; Wed, 12 Jan 2022 10:52:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CA5333858416 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 8BA001F45E; Wed, 12 Jan 2022 10:52:51 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (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 6D178A3B88; Wed, 12 Jan 2022 10:52:51 +0000 (UTC) Date: Wed, 12 Jan 2022 11:52:51 +0100 (CET) From: Richard Biener To: Richard Sandiford cc: "Andre Vieira (lists)" , "gcc-patches@gcc.gnu.org" Subject: Re: [vect] PR103971, PR103977: Fix epilogue mode selection for autodetect only In-Reply-To: Message-ID: <55641q9-6976-r3oq-7r57-2ro73s13qqn@fhfr.qr> References: <9n4q13-4330-20p3-5q25-4rns99s265n8@fhfr.qr> <873456q-1prs-5167-7r7o-966sq485n8p8@fhfr.qr> 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 autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Wed, 12 Jan 2022 10:52:54 -0000 On Wed, 12 Jan 2022, Richard Sandiford wrote: > Richard Biener writes: > > On Wed, 12 Jan 2022, Richard Sandiford wrote: > > > >> Richard Biener writes: > >> > On Wed, 12 Jan 2022, Andre Vieira (lists) wrote: > >> > > >> >> Hi, > >> >> > >> >> This a fix for the regression caused by '[vect] Re-analyze all modes for > >> >> epilogues'. The earlier patch assumed there was always at least one other mode > >> >> than VOIDmode, but that does not need to be the case. > >> >> If we are dealing with a target that does not define more modes for > >> >> 'autovectorize_vector_modes', the behaviour before the patch would be to try > >> >> to create an epilogue for the same autodetected_vector_mode, which unless the > >> >> target supported partial vectors would always fail. So as a fix I suggest > >> >> trying to vectorize the epilogue with the preferred_simd_mode for QI, > >> >> mimicking autovectorize_vector_mode, which will be skipped if it is not a > >> >> vector_mode (since that already should indicate partial vectors aren't > >> >> possible) or if no partial vectors are supported and its pessimistic NUNITS is > >> >> larger than the main loop's VF. > >> >> > >> >> Currently bootstrapping and regression testing, otherwise OK for trunk? Can > >> >> someone verify this fixes the issue for PR103971 on powerpc? > >> > > >> > Why not simply start at mode_i = 0 which means autodetecting the mode > >> > to use for the epilogue? That appears to be a much simpler solution to > >> > me, including for targets where there are more than one element in the > >> > vector. > >> > >> VOIDmode doesn't tell us anything about what the autodetected mode > >> will be, so current short-circuit: > >> > >> /* If the target does not support partial vectors we can shorten the > >> number of modes to analyze for the epilogue as we know we can't pick a > >> mode that has at least as many NUNITS as the main loop's vectorization > >> factor, since that would imply the epilogue's vectorization factor > >> would be at least as high as the main loop's and we would be > >> vectorizing for more scalar iterations than there would be left. */ > >> if (!supports_partial_vectors > >> && maybe_ge (GET_MODE_NUNITS (vector_modes[mode_i]), first_vinfo_vf)) > >> { > >> mode_i++; > >> if (mode_i == vector_modes.length ()) > >> break; > >> continue; > >> } > >> > >> wouldn't be effective. > > > > Well, before this change we simply did > > > > - /* Handle the case that the original loop can use partial > > - vectorization, but want to only adopt it for the epilogue. > > - The retry should be in the same mode as original. */ > > - if (LOOP_VINFO_EPIL_USING_PARTIAL_VECTORS_P (first_loop_vinfo)) > > ... > > - else > > - { > > - mode_i = first_loop_next_i; > > - if (mode_i == vector_modes.length ()) > > - return first_loop_vinfo; > > - } > > > > and thus didn't bother with epilogue vectorization. I think we should > > then just restore this behavior, not doing epilogue vectorization > > if vector_modes.length () == 1? > > Yeah, but that case didn't need epilogue vectorisation before. This > series is adding support for unrolling, and targets with a single vector > size will benefit from epilogues in that case. But in that case (which we could detect), we could then just use autodetected_vector_mode? Like if we do before epilogue vect vector_modes[0] = autodetected_vector_mode; mode_i = 0; thus replace VOIDmode with what we detected and then start at 0? That is, the proposed patch looks very much like a hack to me. I suppose the VECTOR_MODE_P check should be added to if (!supports_partial_vectors && maybe_ge (GET_MODE_NUNITS (vector_modes[mode_i]), first_vinfo_vf)) { mode_i++; instead. > Thanks, > Richard > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)