From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by sourceware.org (Postfix) with ESMTPS id D65B33858C60 for ; Mon, 18 Sep 2023 06:54:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D65B33858C60 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x131.google.com with SMTP id 2adb3069b0e04-502984f5018so6843403e87.3 for ; Sun, 17 Sep 2023 23:54:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695020047; x=1695624847; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=nVqnbbX+BkO9JLHaAUPjX/OxusDyk/WCtWfEBhkNyHE=; b=SxxNhnce2lKgVucUamg7hDH+8kzKe+kyMPnpS+vzVRlxuNtijKZhK8isQLPt7FYglZ z3cjLe7Wso8ftohhold/xxSZS3SfeWV+17G4ge3CaLQBtuaWhN91k2liq83F7uqs/a54 3ZPFUukDlAb2sjTOAcKOAbx0b6SnKOF5rHFRh1CurZNdy37xU9Ea7+U4uVe550+/xJa6 CRXW98oqqHIeLvn8x2w1okGQHuYbNX06L0yuvU+TzvV53MNz+dibEkpj+2UEwWnsQEUX 5G238uujyLlPvlCAz9Yfk/uvlBFsJRBb5XSc591XkwnosVlrvm/YC1vEQbnI7qIhwD3R lh9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695020047; x=1695624847; 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=nVqnbbX+BkO9JLHaAUPjX/OxusDyk/WCtWfEBhkNyHE=; b=lBJjpX68N/q4DOGnmXqRQhM7rpMVOrJvfe0arNmigdynKHh3jBwiLK9KSfjwjtnZQq owdHZTJWAUvdkWwyNR/WXtYdt5JgOqcdBya6rwyGXTi7fNlwRBt6PtXNub4YqGTFlQVn 6qxeL3rYXVpouEBpnopz8Tbdb+Q099NyQUak9RSsf0b7M5KI73l6V1Xwa5UNiaZY5daa aeIb9d29ejTAWW11OvY5fVx2122MYAx4sFBglmSL0bO7jpP3wB8La13E8e+JlcF42+DI p9NbN3MFfznqiyTUNJImdxD8RryhzHH6gRfNdVTALLtzDpmxVKw8u9pm/UYD1X+V+LoP pP1A== X-Gm-Message-State: AOJu0Yz+Qt0vri/FwR9YNLn45DMfOhkwuq+wzT1rS5Ke5UtchRRTn8vQ NNYdl0BRkjZg9/1IKqKBSXUsD7YcA+Mg3McKR3qkpMnW X-Google-Smtp-Source: AGHT+IGi6ICFGz69jvgEApvJFFJ7zAVZAdTQbNzhIMZuMXRSPy8W45xUqjShNinwAXMiU28rrlTi6wkTVwe5wVHq2xo= X-Received: by 2002:a05:6512:3685:b0:500:aed0:cb1b with SMTP id d5-20020a056512368500b00500aed0cb1bmr5689907lfs.24.1695020046892; Sun, 17 Sep 2023 23:54:06 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Mon, 18 Sep 2023 08:53:54 +0200 Message-ID: Subject: Re: [PATCH] [RFC] New early __builtin_unreachable processing. To: Andrew MacLeod Cc: gcc-patches , "hernandez, aldy" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: On Fri, Sep 15, 2023 at 4:45=E2=80=AFPM Andrew MacLeod wrote: > > Ive been looking at __builtin_unreachable () regressions. The > fundamental problem seems to be a lack of consistent expectation for > when we remove it earlier than the final pass of VRP. After looking > through them, I think this provides a sensible approach. > > Ranger is pretty good at providing ranges in blocks dominated by the > __builtin_unreachable branch, so removing it isn't quite a critical as > it once was. Its also pretty good at identifying what in the block can > be affected by the branch. > > This patch provide an alternate removal algorithm for earlier passes. > it looks at *all* the exports from the block, and if the branch > dominates every use of all the exports, AND none of those values access > memory, VRP will remove the unreachable call, rewrite the branch, update > all the values globally, and finally perform the simple DCE on the > branch's ssa-name. This is kind of what it did before, but it wasn't > as stringent on the requirements. > > The memory access check is required because there are a couple of test > cases for PRE in which there is a series of instruction leading to an > unreachable call, and none of those ssa names are ever used in the IL > again. The whole chunk is dead, and we update globals, however > pointlessly. However, one of ssa_names loads from memory, and a later > passes commons this value with a later load, and then the unreachable > call provides additional information about the later load. This is > evident in tree-ssa/ssa-pre-34.c. The only way I see to avoid this > situation is to not remove the unreachable if there is a load feeding it. > > What this does is a more sophisticated version of what DOM does in > all_uses_feed_or_dominated_by_stmt. THe feeding instructions dont have > to be single use, but they do have to be dominated by the branch or be > single use within the branches block.. > > If there are multiple uses in the same block as the branch, this does > not remove the unreachable call. If we could be sure there are no > intervening calls or side effects, it would be allowable, but this a > more expensive checking operation. Ranger gets the ranges right anyway, > so with more passes using ranger, Im not sure we'd see much benefit from > the additional analysis. It could always be added later. > > This fixes at least 110249 and 110080 (and probably others). The only > regression is 93917 for which I changed the testcase to adjust > expectations: > > // PR 93917 > void f1(int n) > { > if(n<0) > __builtin_unreachable(); > f3(n); > } > > void f2(int*n) > { > if(*n<0) > __builtin_unreachable(); > f3 (*n); > } > > We were removing both unreachable calls in VRP1, but only updating the > global values in the first case, meaning we lose information. With the > change in semantics, we only update the global in the first case, but we > leave the unreachable call in the second case now (due to the load from > memory). Ranger still calculates the contextual range correctly as [0, > +INF] in the second case, it just doesn't set the global value until > VRP2 when it is removed. > > Does this seem reasonable? I wonder how this addresses the fundamental issue we always faced in that when we apply the range this range info in itself allows the branch to the __builtin_unreachable () to be statically determined, so when the first VRP pass sets the range the next pass evaluating the condition will remove it (and the guarded __builtin_unreachable ()). In principle there's nothing wrong with that if we don't lose the range info during optimizations, but that unfortunately happens more often than wanted and with the __builtin_unreachable () gone we've lost the ability to re-compute them. I think it's good to explicitly remove the branch at the point we want rather than relying on the "next" visitor to pick up the global range. As I read the patch we now remove __builtin_unreachable () explicitly as soon as possible but don't really address the fundamental issue in any way? > Bootstraps on x86_64-pc-linux-gnu with no regressions. OK? > > Andrew > >