Message ID | oro7es36hr.fsf@lxoliva.fsfla.org |
---|---|
State | Unresolved |
Headers |
Return-Path: <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:3b04:b0:fb:cd0c:d3e with SMTP id c4csp8808036dys; Thu, 14 Dec 2023 11:54:42 -0800 (PST) X-Google-Smtp-Source: AGHT+IGwii1tFPFKUTcVklnXTOiphvIrHTlNu14SAFV1hcXtf9qywVXGgzH3G7F+Mwl26SsEXQeD X-Received: by 2002:a05:6871:a003:b0:1fb:18a6:4853 with SMTP id vp3-20020a056871a00300b001fb18a64853mr11182740oab.99.1702583681959; Thu, 14 Dec 2023 11:54:41 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1702583681; cv=pass; d=google.com; s=arc-20160816; b=zckF/yoMlcVlUBahELt/jSJcSSpAb+vditmTUrM5lLXqlgv3JhkaXcE1zGd2Aym1m7 VN+xJFcm4RYHd5twBEFx/YEn1y9AW1/d6weJTyDBlUKnuO6N1Gre4f+LOWAe6ysQPDvS +4s6im3of/wfXMyC9LHuMJT7X4vJu6kuTIFXQ5P8q/zmhTDOBkyKcrSPMveGy4VbaDqm MazPd+YVq7PLG5Rm/zHL0Bc8xV4+HxkDkP1BJ7V/RRbf0nJhMUf9UZT4WyGmJIrwF/BY OM1wwaAv4hcEO4ZFHkmR/CHOJAP5thVPdJpw26j4gtVaFijlUOrU349F/tShhHRbnWfx semw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=errors-to:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:mime-version:user-agent :message-id:date:organization:subject:cc:to:from:dkim-signature :arc-filter:dmarc-filter:delivered-to; bh=xQ+tGvuitghOQPJwPlVFbzrPsXVVTZIkLcUlQGwYvWc=; fh=o9i0XK2rF1g+e40uA5tpRZiZW6rUHhQJ/DoI6LlA7IY=; b=FE1w7FrkDmrwCWzhka1eSZagn5krpmsJiKExH1eRmcLkRurSpQUAGTJ3L6fARZMqUA koBL3l+7DiO1Mgf3PMLRN4hiLZgjpx2Pc5IHGx9ioMxBxvet1GlKObXDB72AV0gYNH7E oBnJMcngyyr/yWz26NPTZuZSoW5xICkqLUQmSRnoTZ8EmTPBV8MTnVORoQKR3gKb03qO XUla3FZ3OFFIV8EuttHRE3OIDggnGl83fEZSGzw5dv3wdac4pwtWk4azHymFna4AfR80 lRhibbXx3D5PZ+kGbuXr1NKVG4MK6LINEZAjEuigt+Db8pf/3NVNCxOBRG0k+yE2y76T onpQ== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@adacore.com header.s=google header.b=hDqmoyvr; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=adacore.com Received: from server2.sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id m2-20020a05622a118200b0042578f1a3edsi16321884qtk.710.2023.12.14.11.54.41 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Dec 2023 11:54:41 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@adacore.com header.s=google header.b=hDqmoyvr; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=adacore.com Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2BB0C385E038 for <ouuuleilei@gmail.com>; Thu, 14 Dec 2023 19:54:41 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pg1-x529.google.com (mail-pg1-x529.google.com [IPv6:2607:f8b0:4864:20::529]) by sourceware.org (Postfix) with ESMTPS id D62373858C2C for <gcc-patches@gcc.gnu.org>; Thu, 14 Dec 2023 19:54:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D62373858C2C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D62373858C2C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::529 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702583659; cv=none; b=xSOrfkyqvJuI3NQFn9w90zJIBEJ78d7xbCX/eOloVNbE272NS0XkVynd6w4pD1c6ztvoHcN5TFsz5P4tfRhE/I27wJOGOYDOPMY2H0tleFydI6wIpraJnkg+23FQb3jQaX/vDqCMb3JuE0eOTOBSF01I5UT+wPem7LhsxKwD+wk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702583659; c=relaxed/simple; bh=hpT622C+OqEWxR8dptEkx0WAOmAxNDZdeJ41EGMsliY=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=I2R102dPDduKx7DhVtRa7zTGEaePOBAP0KoFwaiAjYR0XSqGnVHLHFSHQIDFFuG2ZhL5/Vz0r75DJGOVNG1UQhfPQ7cJgLxMvpPX7/XiGZFWaLkmkaX7TPU19sELajfpnenmTBhHfvAH6xCzyEAZR4kn5tCSzkaM9kugb8L/jkM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pg1-x529.google.com with SMTP id 41be03b00d2f7-5c69ecda229so4796647a12.2 for <gcc-patches@gcc.gnu.org>; Thu, 14 Dec 2023 11:54:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1702583657; x=1703188457; darn=gcc.gnu.org; h=mime-version:user-agent:message-id:date:organization:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=xQ+tGvuitghOQPJwPlVFbzrPsXVVTZIkLcUlQGwYvWc=; b=hDqmoyvrwLB0l2VFkEexptxdcVZehJ7HR9h64seaki6a6q2t0tB9Y5VwlJiF569rIw fpgGlWnmkSLr8d5l8IhNXCgMZG0jipF+s0YGoiWSx5waqfF/9kbDqVCtDLf2QkbOQh7G X3HqlSugsK4Ra9VvQ+XjoqQy4k5siqmsF7vyJnykyUPEgKzQPITVs/5DsjbtMmH9hpsx Vj3kFahnsFnO/4pr6HpHg9SKhmY+s0YMK0JTUX5A8nsdpVfRPV7tZgYpDG24O8blZDih 6zFyd2RM87vqaf7os32KspexD8C0jthxMGBqw6POMn8zozptZHwUj9wWqztWjldCouLh FVVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702583657; x=1703188457; h=mime-version:user-agent:message-id:date:organization:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=xQ+tGvuitghOQPJwPlVFbzrPsXVVTZIkLcUlQGwYvWc=; b=m6R/L5XMOauuCFgEciFkALmuyjEoaT/epE4VBVY6uuehR4+Cz2oYGFvfvtiooqpy2L rB7QzZtQLsiFxW8nCXZntDgTWInw0uNK7oAQMZJZ06Yk1IIZbXnLChAM7UdXt+mmVTZN Q7Dk5NzDgmLIFNIhZT4eCqz2kS8IeJE/LlC+mHDs3h3HuxY2FRj1Qc+S4Uc2jdjmY07T SIJs3+I3iZHr2Mv+HbyIRpD12ZZL/kaDk/EBQqA186xPJ53Ed8RKF45AwgTQ3dlHIxOL XtRnqkmP7wL7qmi52Am3T4CUCUcJ6X9UN9oRHi5c5GpVPk9mWoOWWg7bn7PvdZ6hsz8G zaNw== X-Gm-Message-State: AOJu0YyL65FcQVnSjDSDra0eUWlPsM8CQxZQX7D4QN9a/vtO53jooXF5 0ol6vKcorgPM3abZPsAmpuAZ+s8KvIKrIxP5SySOrQ== X-Received: by 2002:a05:6a20:144c:b0:181:bc16:48dd with SMTP id a12-20020a056a20144c00b00181bc1648ddmr6786749pzi.47.1702583656819; Thu, 14 Dec 2023 11:54:16 -0800 (PST) Received: from free.home ([2804:7f1:2080:cecc:5977:530e:86b5:e7be]) by smtp.gmail.com with ESMTPSA id r1-20020a62e401000000b006ce91d27c72sm12597422pfh.175.2023.12.14.11.54.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Dec 2023 11:54:16 -0800 (PST) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 3BEJs8OF488980 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 14 Dec 2023 16:54:08 -0300 From: Alexandre Oliva <oliva@adacore.com> To: gcc-patches@gcc.gnu.org Cc: Richard Biener <rguenther@suse.de> Subject: [PATCH] strub: use opt_for_fn during ipa Organization: Free thinker, does not speak for AdaCore Date: Thu, 14 Dec 2023 16:54:08 -0300 Message-ID: <oro7es36hr.fsf@lxoliva.fsfla.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_QUOTING 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.30 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785288386747311079 X-GMAIL-MSGID: 1785288386747311079 |
Series |
strub: use opt_for_fn during ipa
|
|
Checks
Context | Check | Description |
---|---|---|
snail/gcc-patch-check | warning | Git am fail log |
Commit Message
Alexandre Oliva
Dec. 14, 2023, 7:54 p.m. UTC
Instead of global optimization levels and flags, check per-function ones. Regstrapped on x86_64-linux-gnu, also testing on sparc-solaris2.11.3. Ok to install? (sorry, Richi, I dropped the ball and failed to fix this before the monster commit) for gcc/ChangeLog * ipa-strub.cc (gsi_insert_finally_seq_after_call): Likewise. (pass_ipa_strub::adjust_at_calls_call): Likewise. --- gcc/ipa-strub.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Comments
On Thu, 14 Dec 2023, Alexandre Oliva wrote: > > Instead of global optimization levels and flags, check per-function > ones. > > Regstrapped on x86_64-linux-gnu, also testing on sparc-solaris2.11.3. > Ok to install? You have to be generally careful when working within IPA with function bodies without push/pop_cfun around that, several APIs have variants with struct function sepcified, using the wrong one will get you a NULL cfun which _some_ of them handle gracefully and "wrong", all EH stuff is amongst this for example. I see you replace flag_exceptions with opt_for_fn (cfun->decl, flag_exceptions), given that's 'cfun' this replacement is a no-op given 'cfun' would be NULL in IPA context unless you pushed a function. Looking at the 2nd hunk and the caller it seems the transform is a no-op for indrect_calls but not callees, thus that hunk is OK. I'll note that push/pop_cfun can be quite expensive so it's worth avoiding it when possible (we do allow quite some IL manipulation from "outside" of the 'cfun', you just have to be careful with APIs) Thanks, Richard. > (sorry, Richi, I dropped the ball and failed to fix this before the > monster commit) > > > for gcc/ChangeLog > > * ipa-strub.cc (gsi_insert_finally_seq_after_call): Likewise. > (pass_ipa_strub::adjust_at_calls_call): Likewise. > --- > gcc/ipa-strub.cc | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc > index 943bb60996fc1..32e2869cf7840 100644 > --- a/gcc/ipa-strub.cc > +++ b/gcc/ipa-strub.cc > @@ -2132,7 +2132,7 @@ gsi_insert_finally_seq_after_call (gimple_stmt_iterator gsi, gimple_seq seq) > || (call && gimple_call_nothrow_p (call)) > || (eh_lp <= 0 > && (TREE_NOTHROW (cfun->decl) > - || !flag_exceptions))); > + || !opt_for_fn (cfun->decl, flag_exceptions)))); > > if (noreturn_p && nothrow_p) > return; > @@ -2470,9 +2470,11 @@ pass_ipa_strub::adjust_at_calls_call (cgraph_edge *e, int named_args, > /* If we're already within a strub context, pass on the incoming watermark > pointer, and omit the enter and leave calls around the modified call, as an > optimization, or as a means to satisfy a tail-call requirement. */ > - tree swmp = ((optimize_size || optimize > 2 > + tree swmp = ((opt_for_fn (e->caller->decl, optimize_size) > + || opt_for_fn (e->caller->decl, optimize) > 2 > || gimple_call_must_tail_p (ocall) > - || (optimize == 2 && gimple_call_tail_p (ocall))) > + || (opt_for_fn (e->caller->decl, optimize) == 2 > + && gimple_call_tail_p (ocall))) > ? strub_watermark_parm (e->caller->decl) > : NULL_TREE); > bool omit_own_watermark = swmp; > >
On Dec 15, 2023, Richard Biener <rguenther@suse.de> wrote: > You have to be generally careful when working within IPA > with function bodies without push/pop_cfun around that, several APIs > have variants with struct function sepcified, using the wrong one > will get you a NULL cfun which _some_ of them handle gracefully and > "wrong", all EH stuff is amongst this for example. *nod*, I recall running into that, and finding some APIs that required push/pop_cfun, so since I was implementing strub so that it could be plugged into an existing compiler, I didn't give much thought to introducing alternate APIs that could. IIRC I first hit something about EH, and then I had to put in push/pop_cfun. That was very early on, so after that I may have used implicit-cfun APIs without getting ICEs. I suppose now that strub is in pursuing push/pop_cfun avoidance could be a nice cleanup. > I see you replace flag_exceptions with opt_for_fn (cfun->decl, > flag_exceptions), given that's 'cfun' this replacement is a no-op > given 'cfun' would be NULL in IPA context unless you pushed a function. > Looking at the 2nd hunk and the caller it seems the transform is > a no-op for indrect_calls but not callees, thus that hunk is OK. Yeah, I figured that was the reason behind your recommendation, but I guess adding explicit uses of cfun (rather than passing a function around) doesn't really make things much better, except inasmuchas it enables a future de-cfun-ification of strub passes to be a little more mechanical.
On Tue, 19 Dec 2023, Alexandre Oliva wrote: > On Dec 15, 2023, Richard Biener <rguenther@suse.de> wrote: > > > You have to be generally careful when working within IPA > > with function bodies without push/pop_cfun around that, several APIs > > have variants with struct function sepcified, using the wrong one > > will get you a NULL cfun which _some_ of them handle gracefully and > > "wrong", all EH stuff is amongst this for example. > > *nod*, I recall running into that, and finding some APIs that required > push/pop_cfun, so since I was implementing strub so that it could be > plugged into an existing compiler, I didn't give much thought to > introducing alternate APIs that could. IIRC I first hit something about > EH, and then I had to put in push/pop_cfun. That was very early on, so > after that I may have used implicit-cfun APIs without getting ICEs. I > suppose now that strub is in pursuing push/pop_cfun avoidance could be a > nice cleanup. Yeah, adding API variants with explicit struct function is considered OK when that allows to avoid push/pop_cfun. When playing with stmts the first thing you'll hit is update_stmt (though it's core worker, update_stmt_operands has the arg already). Sometimes the cfun dependence isn't really obvious ... > > I see you replace flag_exceptions with opt_for_fn (cfun->decl, > > flag_exceptions), given that's 'cfun' this replacement is a no-op > > given 'cfun' would be NULL in IPA context unless you pushed a function. > > > Looking at the 2nd hunk and the caller it seems the transform is > > a no-op for indrect_calls but not callees, thus that hunk is OK. > > Yeah, I figured that was the reason behind your recommendation, but I > guess adding explicit uses of cfun (rather than passing a function > around) doesn't really make things much better, except inasmuchas it > enables a future de-cfun-ification of strub passes to be a little more > mechanical. Yep, we've gone through some of GCCs APIs where there was two variants, one with and one without explicit struct function arg and eliminating the first in favor of explicit mentioning of 'cfun' to show where we depend on that. That was in the context of eventually threading parts of GCC which of course doesn't play well with this kind of global state. Richard.
diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc index 943bb60996fc1..32e2869cf7840 100644 --- a/gcc/ipa-strub.cc +++ b/gcc/ipa-strub.cc @@ -2132,7 +2132,7 @@ gsi_insert_finally_seq_after_call (gimple_stmt_iterator gsi, gimple_seq seq) || (call && gimple_call_nothrow_p (call)) || (eh_lp <= 0 && (TREE_NOTHROW (cfun->decl) - || !flag_exceptions))); + || !opt_for_fn (cfun->decl, flag_exceptions)))); if (noreturn_p && nothrow_p) return; @@ -2470,9 +2470,11 @@ pass_ipa_strub::adjust_at_calls_call (cgraph_edge *e, int named_args, /* If we're already within a strub context, pass on the incoming watermark pointer, and omit the enter and leave calls around the modified call, as an optimization, or as a means to satisfy a tail-call requirement. */ - tree swmp = ((optimize_size || optimize > 2 + tree swmp = ((opt_for_fn (e->caller->decl, optimize_size) + || opt_for_fn (e->caller->decl, optimize) > 2 || gimple_call_must_tail_p (ocall) - || (optimize == 2 && gimple_call_tail_p (ocall))) + || (opt_for_fn (e->caller->decl, optimize) == 2 + && gimple_call_tail_p (ocall))) ? strub_watermark_parm (e->caller->decl) : NULL_TREE); bool omit_own_watermark = swmp;