| Message ID | 26efeb4d-94df-4cd6-9b8e-318d06136b5e@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [COMMITTED] PR tree-optimization/117398 - Don't call invert on VARYING. | expand |
> Am 04.11.2024 um 16:01 schrieb Andrew MacLeod <amacleod@redhat.com>: > > The invert() range operation is not supported on values of either VARYING or UNDEFINED. Primarily this is because UNDEFINED has no type, which makes it impossible to perform invert() twice on a value, and produce that same value. There were also times when it wasn't precisely clear what the client expects when UNDEFINED or VARYING is inverted.. so it is handled at the caller point. > > When processing switches, whenever we find the ranges for a particular label, we invert that range, and intersect that inversion with the current "default:" ranges to determine the default case. In this testcase, one label has all possible values, so the label was producing VARYING. inverting that value was causing a trap. > > This patch simple checks for VARYING and sets the default ranges to UNDEFINED in this case. Is that correct? The VARYING is not necessarily precise so „the rest“ should be VARYING as well? That said, I don’t think you can use invert and intersect to compute the default case range. Richard > Bootstraps on x86_64-pc-linux-gnu with no regressions. Pushed. > > Backports to GCC12 - 14 forthcoming. > > Andrew > > > <0001-Don-t-call-invert-on-VARYING.patch>
On 11/4/24 10:27, Richard Biener wrote: > >> Am 04.11.2024 um 16:01 schrieb Andrew MacLeod <amacleod@redhat.com>: >> >> The invert() range operation is not supported on values of either VARYING or UNDEFINED. Primarily this is because UNDEFINED has no type, which makes it impossible to perform invert() twice on a value, and produce that same value. There were also times when it wasn't precisely clear what the client expects when UNDEFINED or VARYING is inverted.. so it is handled at the caller point. >> >> When processing switches, whenever we find the ranges for a particular label, we invert that range, and intersect that inversion with the current "default:" ranges to determine the default case. In this testcase, one label has all possible values, so the label was producing VARYING. inverting that value was causing a trap. >> >> This patch simple checks for VARYING and sets the default ranges to UNDEFINED in this case. > Is that correct? The VARYING is not necessarily precise so „the rest“ should be VARYING as well? That said, I don’t think you can use invert and intersect to compute the default case range. The varying *is* precise. It only comes up with varying if all the precise values on each edge unioned together come out to varying. in this case, boolean true and false. proiduces varying, leaving nothing for the default cause. Andrew
> Am 04.11.2024 um 16:32 schrieb Andrew MacLeod <amacleod@redhat.com>: > > >> On 11/4/24 10:27, Richard Biener wrote: >> >>>> Am 04.11.2024 um 16:01 schrieb Andrew MacLeod <amacleod@redhat.com>: >>> >>> The invert() range operation is not supported on values of either VARYING or UNDEFINED. Primarily this is because UNDEFINED has no type, which makes it impossible to perform invert() twice on a value, and produce that same value. There were also times when it wasn't precisely clear what the client expects when UNDEFINED or VARYING is inverted.. so it is handled at the caller point. >>> >>> When processing switches, whenever we find the ranges for a particular label, we invert that range, and intersect that inversion with the current "default:" ranges to determine the default case. In this testcase, one label has all possible values, so the label was producing VARYING. inverting that value was causing a trap. >>> >>> This patch simple checks for VARYING and sets the default ranges to UNDEFINED in this case. >> Is that correct? The VARYING is not necessarily precise so „the rest“ should be VARYING as well? That said, I don’t think you can use invert and intersect to compute the default case range. > > The varying *is* precise. It only comes up with varying if all the precise values on each edge unioned together come out to varying. in this case, boolean true and false. proiduces varying, leaving nothing for the default cause. It never gets VARYING because too many incoming edges and thus conservatively merging subranges? I would doubt ranger can do precise ranges this way. 1 and 5 are conservatively [1, 5] but the inversion does not include 2 and thus is not conservative. Richard > > Andrew >
> >> Am 04.11.2024 um 16:32 schrieb Andrew MacLeod <amacleod@redhat.com>: >> >> >>> On 11/4/24 10:27, Richard Biener wrote: >>> >>>>> Am 04.11.2024 um 16:01 schrieb Andrew MacLeod <amacleod@redhat.com>: >>>> The invert() range operation is not supported on values of either VARYING or UNDEFINED. Primarily this is because UNDEFINED has no type, which makes it impossible to perform invert() twice on a value, and produce that same value. There were also times when it wasn't precisely clear what the client expects when UNDEFINED or VARYING is inverted.. so it is handled at the caller point. >>>> >>>> When processing switches, whenever we find the ranges for a particular label, we invert that range, and intersect that inversion with the current "default:" ranges to determine the default case. In this testcase, one label has all possible values, so the label was producing VARYING. inverting that value was causing a trap. >>>> >>>> This patch simple checks for VARYING and sets the default ranges to UNDEFINED in this case. >>> Is that correct? The VARYING is not necessarily precise so „the rest“ should be VARYING as well? That said, I don’t think you can use invert and intersect to compute the default case range. >> The varying *is* precise. It only comes up with varying if all the precise values on each edge unioned together come out to varying. in this case, boolean true and false. proiduces varying, leaving nothing for the default cause. > It never gets VARYING because too many incoming edges and thus conservatively merging subranges? I would doubt ranger can do precise ranges this way. 1 and 5 are conservatively [1, 5] but the inversion does not include 2 and thus is not conservative. > > Richard > No, that does not happen,at least it should not. If we have too many edges to process (-param=vrp-switch-limit=) , we do not process the switch at all. The limit is because processing massive numbers of subranges can get expensive and ceases to be worthwhile after a while . we've generated the range for default this way since gcc12 at least. Default starts with VARYING, and as we process each edge, we add the ranges on the edge to the destination case, and remove those ranges from the default case (hence invert and intersect). At the end, the range for default contains all the ranges that do not reach a case label. Andrew
On 11/4/24 10:00, Andrew MacLeod wrote: > The invert() range operation is not supported on values of either > VARYING or UNDEFINED. Primarily this is because UNDEFINED has no > type, which makes it impossible to perform invert() twice on a value, > and produce that same value. There were also times when it wasn't > precisely clear what the client expects when UNDEFINED or VARYING is > inverted.. so it is handled at the caller point. > > When processing switches, whenever we find the ranges for a particular > label, we invert that range, and intersect that inversion with the > current "default:" ranges to determine the default case. In this > testcase, one label has all possible values, so the label was > producing VARYING. inverting that value was causing a trap. > > This patch simple checks for VARYING and sets the default ranges to > UNDEFINED in this case. > > Bootstraps on x86_64-pc-linux-gnu with no regressions. Pushed. > > Backports to GCC12 - 14 forthcoming. > > Andrew > > Commited virtuslly identical patch: commit e1154e294b3d8f7267612afb2113b2572cb39e33 (HEAD -> gcc14, origin/releases/gcc-14) Author: Andrew MacLeod <amacleod@redhat.com> Date: Sat Nov 2 10:26:24 2024 -0400 Don't call invert on VARYING. When all cases go to one label and resul in a VARYING value, we can't invert that value to remove all values from the default case. Simply check for this case and set the default to UNDEFINED. PR tree-optimization/117398 gcc/ * gimple-range-edge.cc (gimple_outgoing_range::calc_switch_ranges): Check for VARYING and don't call invert () on it. gcc/testsuite/ * gcc.dg/pr117398.c: New. commit 84b713589646c63b38ec55352fb87c1e80b69b66 (HEAD -> gcc-13) Author: Andrew MacLeod <amacleod@redhat.com> Date: Mon Nov 4 10:02:35 2024 -0500 Don't call invert on VARYING. When all cases go to one label and resul in a VARYING value, we can't invert that value to remove all values from the default case. Simply check for this case and set the default to UNDEFINED. PR tree-optimization/117398 gcc/ * gimple-range-edge.cc (gimple_outgoing_range::calc_switch_ranges): Check for VARYING and don't call invert () on it. gcc/testsuite/ * gcc.dg/pr117398.c: New. commit 4b9d6952d4024c5b8a66146ad497d63ad76a4095 (HEAD -> gcc12) Author: Andrew MacLeod <amacleod@redhat.com> Date: Mon Nov 4 10:02:35 2024 -0500 Don't call invert on VARYING. When all cases go to one label and resul in a VARYING value, we can't invert that value to remove all values from the default case. Simply check for this case and set the default to UNDEFINED. PR tree-optimization/117398 gcc/ * gimple-range-edge.cc (gimple_outgoing_range::calc_switch_ranges): Check for VARYING and don't call invert () on it. gcc/testsuite/ * gcc.dg/pr117398.c: New.
From 766075c47db5cc9d04463bfb2219b593bb4263ee Mon Sep 17 00:00:00 2001 From: Andrew MacLeod <amacleod@redhat.com> Date: Sat, 2 Nov 2024 10:26:24 -0400 Subject: [PATCH] Don't call invert on VARYING. When all cases go to one label and resul in a VARYING value, we can't invert that value to remove all values from the default case. Simply check for this case and set the default to UNDEFINED. PR tree-optimization/117398 gcc/ * gimple-range-edge.cc (gimple_outgoing_range::calc_switch_ranges): Check for VARYING and don't call invert () on it. gcc/testsuite/ * gcc.dg/pr117398.c: New. --- gcc/gimple-range-edge.cc | 10 ++++++++-- gcc/testsuite/gcc.dg/pr117398.c | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr117398.c diff --git a/gcc/gimple-range-edge.cc b/gcc/gimple-range-edge.cc index e3a197a2293..d2387289ad2 100644 --- a/gcc/gimple-range-edge.cc +++ b/gcc/gimple-range-edge.cc @@ -159,8 +159,14 @@ gimple_outgoing_range::calc_switch_ranges (gswitch *sw) // Remove the case range from the default case. int_range_max def_range (type, low, high); range_cast (def_range, type); - def_range.invert (); - default_range.intersect (def_range); + // If all possible values are taken, set default_range to UNDEFINED. + if (def_range.varying_p ()) + default_range.set_undefined (); + else + { + def_range.invert (); + default_range.intersect (def_range); + } // Create/union this case with anything on else on the edge. int_range_max case_range (type, low, high); diff --git a/gcc/testsuite/gcc.dg/pr117398.c b/gcc/testsuite/gcc.dg/pr117398.c new file mode 100644 index 00000000000..c43f2a3ed6b --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr117398.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +int a; +void c(void); +int d(_Bool b) { + switch (b+0) { + case 0: + break; + case 1: + break; + default: + c(); + } + if (b) + return a; +} -- 2.45.0