From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id EE8DC3858D35 for ; Tue, 4 Jul 2023 11:52:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EE8DC3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 0E66620539; Tue, 4 Jul 2023 11:52:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1688471531; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=+ooIOIjnE0Sll3c6F4VX0EtEsp/WN/6aphF1u1CEMn4=; b=baHM3vSaCac0NDJHF8oj8By2wPeA7dpfjBUBeGaN4hF5DUkXZMtKiVFVN/+cxRemHQyftL x01NOKcVfSFWZSDkee7qSkJPGFBUEOYQU+nKVJu7qhbyqQED9j7M42l0XaVV5x3SBlIGTY 54cd/r3hfHhvcAEgbpf1aZIoIn8Wq08= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1688471531; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=+ooIOIjnE0Sll3c6F4VX0EtEsp/WN/6aphF1u1CEMn4=; b=RqFOPVOs7U5l5QlmsrLBg+d6mErgwEO8MvqFrqHMWolVuE1AhBVRJbz/YNdJxfgBT8D77E iD5cEzjIGhtdaGBA== 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 E72C72C141; Tue, 4 Jul 2023 11:52:10 +0000 (UTC) Date: Tue, 4 Jul 2023 11:52:10 +0000 (UTC) From: Richard Biener To: Tamar Christina cc: gcc-patches@gcc.gnu.org, nd@arm.com, jlaw@ventanamicro.com, Jan Hubicka Subject: Re: [PATCH 4/19]middle-end: Fix scale_loop_frequencies segfault on multiple-exits 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=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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 List-Id: On Wed, 28 Jun 2023, Tamar Christina wrote: > Hi All, > > There's an existing bug in loop frequency scaling where the if statement checks > to see if there's a single exit, and records an dump file note but then > continues. > > It then tries to access the null pointer, which of course fails. > > For multiple loop exists it's not really clear how to scale the exit > probablities as it's really unknown which exit is most probably. > > For that reason I ignore the exit edges during scaling but still adjust the > loop body. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? I can't really make sense of /* If latch exists, change its count, since we changed probability of exit. Theoretically we should update everything from source of exit edge to latch, but for vectorizer this is enough. */ if (loop->latch && loop->latch != e->src) loop->latch->count += count_delta; since with simple latches the latch itself is an empty forwarder and e->src is the block with the conditional eventually exiting the block. That means this condition is always true. So I think for exits the idea is to "remove" them by redirecting them "count-wise" back into the loop. So turn if (cond) --(exit-count)-- exit | | in-loop-count | latch into [cond-blk-count] if (cond) -- (zero count) -- exit | | in-loop-cound + exit-count (== cond-blk-count) | latch (now with cond-blk-count) and the comment correctly suggests all blocks following from here would need similar adjustment (and on in-loop branches the delta would be distributed according to probabilities). Given the code is quite imperfect I would suggest to change the updating of the latch block count to read profile_count count_delta = profile_count::zero (); if (loop->latch && single_pred_p (loop->latch) && loop_exits_from_bb_p (single_pred (loop->latch))) { count_delta = single_pred (loop->latch)->count - loop->latch->count; loop->latch->count = single_pred (loop->latch)->count; } scale_loop_frequencies (loop, p); if (count_delta != 0) loop->latch->count -= count_delta; which should exactly preserve the exit-before-latch behavior independent on the number of exits of the loop. Please leave Honza a chance to comment here. Thanks, Richard. > Thanks, > Tamar > > gcc/ChangeLog: > > * cfgloopmanip.cc (scale_loop_frequencies): Fix typo. > (scale_loop_profile): Don't access null pointer. > > --- inline copy of patch -- > diff --git a/gcc/cfgloopmanip.cc b/gcc/cfgloopmanip.cc > index 6e09dcbb0b1864bc64ffd570a4b923f50c3819b5..b10ef3d2be82902ccd74e52a4318217b2db13bcb 100644 > --- a/gcc/cfgloopmanip.cc > +++ b/gcc/cfgloopmanip.cc > @@ -501,7 +501,7 @@ scale_loop_frequencies (class loop *loop, profile_probability p) > /* Scale profile in LOOP by P. > If ITERATION_BOUND is non-zero, scale even further if loop is predicted > to iterate too many times. > - Before caling this function, preheader block profile should be already > + Before calling this function, preheader block profile should be already > scaled to final count. This is necessary because loop iterations are > determined by comparing header edge count to latch ege count and thus > they need to be scaled synchronously. */ > @@ -597,14 +597,14 @@ scale_loop_profile (class loop *loop, profile_probability p, > /* If latch exists, change its count, since we changed > probability of exit. Theoretically we should update everything from > source of exit edge to latch, but for vectorizer this is enough. */ > - if (loop->latch && loop->latch != e->src) > + if (e && loop->latch && loop->latch != e->src) > loop->latch->count += count_delta; > > /* Scale the probabilities. */ > scale_loop_frequencies (loop, p); > > /* Change latch's count back. */ > - if (loop->latch && loop->latch != e->src) > + if (e && loop->latch && loop->latch != e->src) > loop->latch->count -= count_delta; > > if (dump_file && (dump_flags & TDF_DETAILS)) > > > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)