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 ABEEF3858404 for ; Sat, 11 May 2024 09:43:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org ABEEF3858404 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 ABEEF3858404 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=1715420623; cv=none; b=rGqcEyA+sRZZrm5i2/aAbfXFV46qBqBuP8+zmp9ekCuyngtQtReiWKVja1Z2uDRksuqHY+yTX1VMGXiwSSnVy+vgfPJK9FLgbSp3aZo5eh5pQluR6OG56CezeehntHhCdFK66Ude1Y9mH56JrdmgDF5wKzdaWJCYRNrUotIwo4w= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715420623; c=relaxed/simple; bh=ZFQRfmzu1M3uVUoz5YjYXVE/F7XcadkoDGbtzYFDCio=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=N2wjBzoNWrOusHqvz0GIjaOPRG86yQnL9Y1sUkvBk5kLoebDPDHA98K0rl90NHhjwyFUcOxcvAKiKzmDCmXOetBHPJx+3a1X7Ltn+79Mps+kboSn1kisz0ZHhf/q63PtPHnCf0k5De+zTb9BS9axvFU7Bo3RC+N9BJlfaI03L68= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1715420620; 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=APmK7qko9/cVSE1qgqmdo34ddk+NPoZRb0xMV9ShE+0=; b=TZlIks5dqZiVk+eaQX7sR1qa98j4g3uPYhkjBz16EI0e0pzep3KDeOSLuy4ZD8WXbKZ0vS Y4FxW9N/AnBHFETdsoTcMSfzrQmXOF0Kj1NjKi+m4u/4l9To3eP/VnYnag+nPqScAgiarP lmdU0zJXDvr5villLuB7kI4HbN0mFIo= Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-634-NpsUMm2bMduSs4KoK7li7g-1; Sat, 11 May 2024 05:43:38 -0400 X-MC-Unique: NpsUMm2bMduSs4KoK7li7g-1 Received: by mail-ed1-f69.google.com with SMTP id 4fb4d7f45d1cf-5729fd97df9so957404a12.1 for ; Sat, 11 May 2024 02:43:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715420618; x=1716025418; 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=APmK7qko9/cVSE1qgqmdo34ddk+NPoZRb0xMV9ShE+0=; b=XW8+AMWiAvIMbgGbBYZdoQY/P/3whIoYyzacAnqFLOezlpmtFNUQi5kazCzeIrhl3c CNDMGHNWiDbrEjgy1rW3JWzBN2SdGYta0qWNUFrvGCVfqYpTBtUmpcuvb0gO2Q8W1/oc 4KVMycAKavjHGxQPJ6g5qjLzwFrBYHyN+1qPVOvK1x6Fk64xtotcp4nb7QiZMO/I/qbU 7EzT3T1panHDH9/gv5u2JcTsudLMqcM8yVEcJ9W9EESJbXoKLqqDTIY73LYbZaNk8I6W 2GMZiT5aFnZWAnWGkYfmiXzzUR7rhzNtxdnNgeXj/O7ls6UqHsDfWBvbOWv7QCEkS4UN C7wQ== X-Gm-Message-State: AOJu0YyZoVd1i0uRtpJ9Y3C5/WBC0iXMKQRlWvkPTy19LENvCCUZHbLH EXnFed9KhBXfWbTWgcpcm5j1gy5FW6qBisZdT/L+JsWKrhQJlIbKtijrVXvjol0BqpJLmpqplK/ 7m9UAIXGCoWRy+LwQPpoiJuq/+Alb/idDsGCmbC9zUPYFoGF4MVAAQOs9ugsm6kn4ifFtsZvynh KxeX0Xpc45NHmybVsjNcP/SmDxHByjFw== X-Received: by 2002:a50:8747:0:b0:56f:e71b:74e3 with SMTP id 4fb4d7f45d1cf-5734d6f33c0mr3461389a12.39.1715420617723; Sat, 11 May 2024 02:43:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHSK2Dg2f85oC2IZakhnCJtWhF0fxQZV8+iz5K0z0cD+XfSlH5F5RsoGfrmddD+MaN9cPP2FQe/+GCcbZoKqjA= X-Received: by 2002:a50:8747:0:b0:56f:e71b:74e3 with SMTP id 4fb4d7f45d1cf-5734d6f33c0mr3461379a12.39.1715420617309; Sat, 11 May 2024 02:43:37 -0700 (PDT) MIME-Version: 1.0 References: <20240510092417.432674-1-aldyh@redhat.com> In-Reply-To: From: Aldy Hernandez Date: Sat, 11 May 2024 11:43:26 +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.1 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 have pushed a few cleanups to make it easier to move forward without disturbing passes which are affected by IPA's mixing up the range types. As I explained in my previous patch, this restores the default behavior of silently returning VARYING when a range operator is unsupported in either a particular operator, or in the dispatch code. I would like to re-enable prange support, as IPA was already broken before the prange work, and the debugging trap can be turned off to analyze (#define TRAP_ON_UNHANDLED_POINTER_OPERATORS 1). I have re-tested the effects of re-enabling prange in current trunk: 1. x86-64/32 bootstraps with no regressions with and without the trap. 2. ppc64le bootstraps with no regressions, but fails with the trap. 3. aarch64 bootstraps, but fails with the trap (no space on compile farm to run tests) 4. sparc: bootstrap already broken, so I can't test. So, for the above 4 architectures things work as before, and we have a PR to track the IPA problem which doesn't seem to affect neither bootstrap nor tests. Does this sound reasonable? 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 > > >