From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 28D13384CBA1 for ; Fri, 10 May 2024 10:30:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 28D13384CBA1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 28D13384CBA1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715337013; cv=none; b=Hxc0duTPvxX7dE5HVDsJI6dmbvXzQ2asG5/G49GaODqNbH58Y+YIsdM9ZAKfHw/kMr4uV2rctKew8VNKW2ZkH3ynSM/BLvT/3xK+7ttRHYYdUZCsUnHchE/hJKYPgjb2oP9w7mkL/PpNFYpSfYij2AsK5h1PDJteveZMfQw/R8E= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715337013; c=relaxed/simple; bh=zYjdC5VuPOeO1er1ktWCFmNcRXG8m523SsuqInYZSwk=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=xZC5eWFCAO2BBz/DXj+UL0sc94OFAEr77xpEBRrtwBqHEz7ZmljZTbkCByRjt144MxRZ1SznBciK+GLDj2+U3O4HGu20S3mx29ta4g9MknC8i7xy7dJjrrJCFxTreWPW0xj/Kuvl50v0DXns0gyzMSsnqszuo9RUjMJ/KbSdQaA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1715337010; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=t1oZFB0cdL72eOUDanTJc9BRSRmdPBFYKCofv2KhMts=; b=dD8vYMueODe/EVE3te0qdUMTm7Zpl2960LYWb2hV+J5B8CiaBX6xDwLJ3FiXF1II5wwxMX CX3yzks1o+R5B3Iz09pYkU4rNaTbdqos2hyL9o/VEp1UGKpYVJalq1SP2hXLtaA+jyiMnB 3oqpzsjFhWHRnmUd0pXXSyBk8H9bfRM= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-504-9MlnT6YNNKCzrZw0kHiYgA-1; Fri, 10 May 2024 06:30:06 -0400 X-MC-Unique: 9MlnT6YNNKCzrZw0kHiYgA-1 Received: by mail-ej1-f69.google.com with SMTP id a640c23a62f3a-a51fdbd06c8so128144966b.3 for ; Fri, 10 May 2024 03:30:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715337005; x=1715941805; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=t1oZFB0cdL72eOUDanTJc9BRSRmdPBFYKCofv2KhMts=; b=kCsnm6sDJ5RvW3m4hqN4x5/IRocQlJ8K75VAXiRY/2vUw7gZyMQjwoj2bQ5qyCfojs +pKMmtkffe+t6wH7wseOryTb3HxMZB4nJgX4bJlmJCpFVgl9b4pMoWcWBD85VYVTIeBy 5A65o9arNcsTQhjtDG/8fETFFDvQD9cgd4awOuudDg3jdR3bIrS89J9oBn23F9oCpu3b AUQ6HTCC5l0t8qtBwB7gG/nvwzveC59w4F+bksdDm6vpy6A2BV/DJNAYLRA3/6vuNri8 vauC4bmZEyRAvBs5Dvr9vUyzWecIYqWcSMgHFDttFRGWDTeId64bB66Hsrc1cQLEwVJ/ dqbw== X-Gm-Message-State: AOJu0Ywj28pM3du1fYVmUQMgi87tNWMZME4fu7zg4q1XLSdis0dE3jhK o3A3B3bgtV/kydtsHh3dhztxYbKIjGbiBaRBhp3rHNw1gqkG+BJY+IpehTMOxXcSj38eLl1HOlw hjcNFqj1+AlAjAgYhUZ3wgWO/DKYXwZgPThYdeftKXWUZkAAP9X+m/Y9UkGm3v6w+23k1RxdGHJ +KnFxvQrMyC1wQhwSKyp6ci39/Kklrvj/NhPPXXA== X-Received: by 2002:a17:907:86a8:b0:a59:a282:5db7 with SMTP id a640c23a62f3a-a5a2d67e024mr177313466b.65.1715337004936; Fri, 10 May 2024 03:30:04 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHLLRXAHVx2XJ35cfcE4TQsLmIovicLMtihnRhsHJZHBQYxjkfupYgGNwplhm83Qu0zGxrYm2meO/Vph3IrGCE= X-Received: by 2002:a17:907:86a8:b0:a59:a282:5db7 with SMTP id a640c23a62f3a-a5a2d67e024mr177311966b.65.1715337004504; Fri, 10 May 2024 03:30:04 -0700 (PDT) MIME-Version: 1.0 References: <20240510092417.432674-1-aldyh@redhat.com> In-Reply-To: From: Aldy Hernandez Date: Fri, 10 May 2024 12:29:53 +0200 Message-ID: Subject: Re: [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] To: Richard Biener Cc: GCC patches , Martin Jambor , Andrew MacLeod X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,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: I would much prefer the IPA experts to fix the pass, but I'm afraid I don't understand the code enough to do so. Could someone lend a hand here? Aldy On Fri, May 10, 2024 at 12:26=E2=80=AFPM Richard Biener wrote: > > On Fri, May 10, 2024 at 11:24=E2=80=AFAM Aldy Hernandez wrote: > > > > There are various calls into fold_range() that have the wrong type > > associated with the range temporary used to hold the result. This > > used to work, because we could store either integers or pointers in a > > Value_Range, but is no longer the case with prange's. Now you must > > explicitly state which type of range the temporary will hold before > > storing into it. You can change this at a later time with set_type(), > > but you must always have a type before using the temporary, and it > > must match what fold_range() returns. > > > > This patch adjusts the IPA code to restore the previous functionality, > > so I can re-enable the prange code, but I do question whether the > > previous code was correct. I have added appropriate comments to help > > the maintainers, but someone with more knowledge should revamp this > > going forward. > > > > The basic problem is that pointer comparisons return a boolean, but > > the IPA code is initializing the resulting range as a pointer. This > > wasn't a problem, because fold_range() would previously happily force > > the range into an integer one, and everything would work. But now we > > must initialize the range to an integer before calling into > > fold_range. The thing is, that the failing case sets the result back > > into a pointer, which is just weird but existing behavior. I have > > documented this in the code. > > > > if (!handler > > || !op_res.supports_type_p (vr_type) > > || !handler.fold_range (op_res, vr_type, srcvr, op_vr)) > > /* For comparison operators, the type here may be > > different than the range type used in fold_range above. > > For example, vr_type may be a pointer, whereas the type > > returned by fold_range will always be a boolean. > > > > This shouldn't cause any problems, as the set_varying > > below will happily change the type of the range in > > op_res, and then the cast operation in > > ipa_vr_operation_and_type_effects will ultimately leave > > things in the desired type, but it is confusing. > > > > Perhaps the original intent was to use the type of > > op_res here? */ > > op_res.set_varying (vr_type); > > > > BTW, this is not to say that the original gimple IR was wrong, but that > > IPA is setting the range type of the result of fold_range() to the type= of > > the operands, which does not necessarily match in the case of a > > comparison. > > > > I am just restoring previous behavior here, but I do question whether i= t > > was right to begin with. > > > > Testing currently in progress on x86-64 and ppc64le with prange enabled= . > > > > OK pending tests? > > I think this "intermediate" patch is unnecessary and instead the code sho= uld > be fixed correctly, avoiding missed-optimization regressions. > > Richard. > > > gcc/ChangeLog: > > > > PR tree-optimization/114985 > > * ipa-cp.cc (ipa_value_range_from_jfunc): Adjust type of op_res= . > > (propagate_vr_across_jump_function): Same. > > * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Adjust > > type for res. > > * ipa-prop.h (ipa_type_for_fold_range): New. > > --- > > gcc/ipa-cp.cc | 18 ++++++++++++++++-- > > gcc/ipa-fnsummary.cc | 6 +++++- > > gcc/ipa-prop.h | 13 +++++++++++++ > > 3 files changed, 34 insertions(+), 3 deletions(-) > > > > diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc > > index 5781f50c854..3c395632364 100644 > > --- a/gcc/ipa-cp.cc > > +++ b/gcc/ipa-cp.cc > > @@ -1730,7 +1730,7 @@ ipa_value_range_from_jfunc (vrange &vr, > > } > > else > > { > > - Value_Range op_res (vr_type); > > + Value_Range op_res (ipa_type_for_fold_range (operation, vr_ty= pe)); > > Value_Range res (vr_type); > > tree op =3D ipa_get_jf_pass_through_operand (jfunc); > > Value_Range op_vr (TREE_TYPE (op)); > > @@ -1741,6 +1741,19 @@ ipa_value_range_from_jfunc (vrange &vr, > > if (!handler > > || !op_res.supports_type_p (vr_type) > > || !handler.fold_range (op_res, vr_type, srcvr, op_vr)) > > + /* For comparison operators, the type here may be > > + different than the range type used in fold_range above. > > + For example, vr_type may be a pointer, whereas the type > > + returned by fold_range will always be a boolean. > > + > > + This shouldn't cause any problems, as the set_varying > > + below will happily change the type of the range in > > + op_res, and then the cast operation in > > + ipa_vr_operation_and_type_effects will ultimately leave > > + things in the desired type, but it is confusing. > > + > > + Perhaps the original intent was to use the type of > > + op_res here? */ > > op_res.set_varying (vr_type); > > > > if (ipa_vr_operation_and_type_effects (res, > > @@ -2540,7 +2553,7 @@ propagate_vr_across_jump_function (cgraph_edge *c= s, ipa_jump_func *jfunc, > > { > > tree op =3D ipa_get_jf_pass_through_operand (jfunc); > > Value_Range op_vr (TREE_TYPE (op)); > > - Value_Range op_res (param_type); > > + Value_Range op_res (ipa_type_for_fold_range (operation, param= _type)); > > range_op_handler handler (operation); > > > > ipa_range_set_and_normalize (op_vr, op); > > @@ -2549,6 +2562,7 @@ propagate_vr_across_jump_function (cgraph_edge *c= s, ipa_jump_func *jfunc, > > || !ipa_supports_p (operand_type) > > || !handler.fold_range (op_res, operand_type, > > src_lats->m_value_range.m_vr, op_= vr)) > > + /* See note in ipa_value_range_from_jfunc. */ > > op_res.set_varying (param_type); > > > > ipa_vr_operation_and_type_effects (vr, > > diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc > > index 07a853f78e3..4deba2394f5 100644 > > --- a/gcc/ipa-fnsummary.cc > > +++ b/gcc/ipa-fnsummary.cc > > @@ -502,7 +502,8 @@ evaluate_conditions_for_known_args (struct cgraph_n= ode *node, > > if (vr.varying_p () || vr.undefined_p ()) > > break; > > > > - Value_Range res (op->type); > > + Value_Range res (ipa_type_for_fold_range (op->code, > > + op->type)); > > if (!op->val[0]) > > { > > Value_Range varying (op->type); > > @@ -511,6 +512,7 @@ evaluate_conditions_for_known_args (struct cgraph_n= ode *node, > > if (!handler > > || !res.supports_type_p (op->type) > > || !handler.fold_range (res, op->type, vr, va= rying)) > > + /* See note in ipa_value_from_jfunc. */ > > res.set_varying (op->type); > > } > > else if (!op->val[1]) > > @@ -525,9 +527,11 @@ evaluate_conditions_for_known_args (struct cgraph_= node *node, > > || !handler.fold_range (res, op->type, > > op->index ? op0 : vr, > > op->index ? vr : op0)= ) > > + /* See note in ipa_value_from_jfunc. */ > > res.set_varying (op->type); > > } > > else > > + /* See note in ipa_value_from_jfunc. */ > > res.set_varying (op->type); > > vr =3D res; > > } > > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h > > index 93d1b87b1f7..8493dd19b92 100644 > > --- a/gcc/ipa-prop.h > > +++ b/gcc/ipa-prop.h > > @@ -1277,6 +1277,19 @@ ipa_range_set_and_normalize (vrange &r, tree val= ) > > r.set (val, val); > > } > > > > +/* Return the resulting type for a fold_range() operation for OP and > > + TYPE. */ > > + > > +inline tree > > +ipa_type_for_fold_range (tree_code op, tree type) > > +{ > > + /* Comparisons return boolean regardless of their input operands. *= / > > + if (TREE_CODE_CLASS (op) =3D=3D tcc_comparison) > > + return boolean_type_node; > > + > > + return type; > > +} > > + > > bool ipa_return_value_range (Value_Range &range, tree decl); > > void ipa_record_return_value_range (Value_Range val); > > bool ipa_jump_functions_equivalent_p (ipa_jump_func *jf1, ipa_jump_fun= c *jf2); > > -- > > 2.45.0 > > >