Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)
On Fri, Dec 16, 2016 at 03:16:14PM +0100, Marek Polacek wrote:
> On Fri, Dec 16, 2016 at 02:58:59PM +0100, Richard Biener wrote:
> > On Fri, Dec 16, 2016 at 2:03 PM, Bernd Schmidt wrote:
> > > On 12/16/2016 12:49 PM, Marek Polacek wrote:
> > >
> > >> But as this testcase shows, this breaks when the default label shares a
> > >> label
> > >> with another case. On this testcase, when we reach the switch, we know
> > >> that
> > >> argc is either 1, 2, or 3. So by the time we reach vrp2, the IR will
> > >> have
> > >> been optimized to
> > >>
> > >> switch (argc) {
> > >> default:
> > >> case 3:
> > >> // argc will be considered 1 despite the case 3
> > >> break;
> > >> case 2:
> > >> ...
> > >>}
> > >
> > >
> > > Shouldn't we just remove the "case 3:" from the switch in this case? Would
> > > that fix things?
> >
> > We probably should indeed. But can we rely on this?
>
> I think we should do both -- apply my fix + investigated why we kept case 3
> around. I'm willing to look into this.
This is work of simplify_switch_using_ranges and then
11293 /* Update SWITCH_EXPR case label vector. */
11294 FOR_EACH_VEC_ELT (to_update_switch_stmts, i, su)
11295 {
11296 size_t j;
11297 size_t n = TREE_VEC_LENGTH (su->vec);
11298 tree label;
11299 gimple_switch_set_num_labels (su->stmt, n);
11300 for (j = 0; j < n; j++)
11301 gimple_switch_set_label (su->stmt, j, TREE_VEC_ELT (su->vec, j));
11302 /* As we may have replaced the default label with a regular one
11303 make sure to make it a real default label again. This ensures
11304 optimal expansion. */
11305 label = gimple_switch_label (su->stmt, 0);
11306 CASE_LOW (label) = NULL_TREE;
11307 CASE_HIGH (label) = NULL_TREE;
^^ this
But only after I wrote a patch I noticed that cfgcleanup merges default: and
case 3: before we reach .optimized, so I think there's nothing to do here.
Marek
Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)
On Fri, Dec 16, 2016 at 3:16 PM, Marek Polacek wrote:
> On Fri, Dec 16, 2016 at 02:58:59PM +0100, Richard Biener wrote:
>> On Fri, Dec 16, 2016 at 2:03 PM, Bernd Schmidt wrote:
>> > On 12/16/2016 12:49 PM, Marek Polacek wrote:
>> >
>> >> But as this testcase shows, this breaks when the default label shares a
>> >> label
>> >> with another case. On this testcase, when we reach the switch, we know
>> >> that
>> >> argc is either 1, 2, or 3. So by the time we reach vrp2, the IR will have
>> >> been optimized to
>> >>
>> >> switch (argc) {
>> >> default:
>> >> case 3:
>> >> // argc will be considered 1 despite the case 3
>> >> break;
>> >> case 2:
>> >> ...
>> >>}
>> >
>> >
>> > Shouldn't we just remove the "case 3:" from the switch in this case? Would
>> > that fix things?
>>
>> We probably should indeed. But can we rely on this?
>
> I think we should do both -- apply my fix + investigated why we kept case 3
> around. I'm willing to look into this.
Agreed. (my original approval still holds)
Richard.
> Marek
Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)
On Fri, Dec 16, 2016 at 02:58:59PM +0100, Richard Biener wrote:
> On Fri, Dec 16, 2016 at 2:03 PM, Bernd Schmidt wrote:
> > On 12/16/2016 12:49 PM, Marek Polacek wrote:
> >
> >> But as this testcase shows, this breaks when the default label shares a
> >> label
> >> with another case. On this testcase, when we reach the switch, we know
> >> that
> >> argc is either 1, 2, or 3. So by the time we reach vrp2, the IR will have
> >> been optimized to
> >>
> >> switch (argc) {
> >> default:
> >> case 3:
> >> // argc will be considered 1 despite the case 3
> >> break;
> >> case 2:
> >> ...
> >>}
> >
> >
> > Shouldn't we just remove the "case 3:" from the switch in this case? Would
> > that fix things?
>
> We probably should indeed. But can we rely on this?
I think we should do both -- apply my fix + investigated why we kept case 3
around. I'm willing to look into this.
Marek
Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)
On Fri, Dec 16, 2016 at 2:03 PM, Bernd Schmidt wrote:
> On 12/16/2016 12:49 PM, Marek Polacek wrote:
>
>> But as this testcase shows, this breaks when the default label shares a
>> label
>> with another case. On this testcase, when we reach the switch, we know
>> that
>> argc is either 1, 2, or 3. So by the time we reach vrp2, the IR will have
>> been optimized to
>>
>> switch (argc) {
>> default:
>> case 3:
>> // argc will be considered 1 despite the case 3
>> break;
>> case 2:
>> ...
>>}
>
>
> Shouldn't we just remove the "case 3:" from the switch in this case? Would
> that fix things?
We probably should indeed. But can we rely on this?
Richard.
>
> Bernd
Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)
On 12/16/2016 12:49 PM, Marek Polacek wrote:
But as this testcase shows, this breaks when the default label shares a label
with another case. On this testcase, when we reach the switch, we know that
argc is either 1, 2, or 3. So by the time we reach vrp2, the IR will have
been optimized to
switch (argc) {
default:
case 3:
// argc will be considered 1 despite the case 3
break;
case 2:
...
}
Shouldn't we just remove the "case 3:" from the switch in this case?
Would that fix things?
Bernd
Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)
On Fri, Dec 16, 2016 at 02:00:56PM +0100, Richard Biener wrote:
> On Fri, Dec 16, 2016 at 12:49 PM, Marek Polacek wrote:
> > --- gcc/tree-vrp.c
> > +++ gcc/tree-vrp.c
> > @@ -6051,10 +6051,17 @@ find_switch_asserts (basic_block bb, gswitch *last)
> >/* Now register along the default label assertions that correspond to the
> > anti-range of each label. */
> >int insertion_limit = PARAM_VALUE (PARAM_MAX_VRP_SWITCH_ASSERTIONS);
> > + if (insertion_limit == 0)
> > +return;
> > +
> > + /* We can't do this if the default case shares a label with another
> > case. */
> > + tree default_cl = gimple_switch_default_label (last);
> >for (idx = 1; idx < n; idx++)
> > {
> >tree min, max;
> >tree cl = gimple_switch_label (last, idx);
> > + if (CASE_LABEL (cl) == CASE_LABEL (default_cl))
> > + break;
>
> It's conservative to break here but don't you actually want to continue
> instead?
Right, that should be safe and generate better range info for the default
label.
> >min = CASE_LOW (cl);
> >max = CASE_HIGH (cl);
> > @@ -6065,6 +6072,8 @@ find_switch_asserts (basic_block bb, gswitch *last)
> > {
> > tree next_min, next_max;
> > tree next_cl = gimple_switch_label (last, idx);
> > + if (CASE_LABEL (next_cl) == CASE_LABEL (default_cl))
> > + break;
>
> here the break is of course correct.
>
> Ok with that change.
Thanks,
Marek
Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)
On Fri, Dec 16, 2016 at 12:49 PM, Marek Polacek wrote:
> Starting with r238761, we register assertions for default switch labels;
> these assertions correspond to the anti-range of each case label. It
> means that we can optimize this better:
>
> switch (i) {
> case 1: case 2: case 3:
> ...
> break;
> default:
> // i is ~[1, 3]
> if (i == 2) // fold to 0
> ...
> }
>
> But as this testcase shows, this breaks when the default label shares a label
> with another case. On this testcase, when we reach the switch, we know that
> argc is either 1, 2, or 3. So by the time we reach vrp2, the IR will have
> been optimized to
>
> switch (argc) {
> default:
> case 3:
> // argc will be considered 1 despite the case 3
> break;
> case 2:
> ...
>}
>
> Now, the assertion for argc in default: is that it's ~[2,3]. Since argc had
> been proved to be either 1, 2, or 3, we optimized it to 1. Given the
> consecutive case 3: this is incorrect.
>
> So ignore the range when a case label is "shared" with the default case.
>
> Another bug: --param max-vrp-switch-assertions=0 didn't work -- we register
> the assertion and then there's this check
>
> if (--insertion_limit == 0)
> break;
>
> but given the decrement this isn't true so we'd kept going.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2016-12-16 Marek Polacek
>
> PR tree-optimization/78819
> * tree-vrp.c (find_switch_asserts): Return if the insertion limit is
> 0.
> Don't register an assertion if the default case shares a label with
> another case.
>
> * gcc.dg/tree-ssa/vrp112.c: New test.
>
> diff --git gcc/testsuite/gcc.dg/tree-ssa/vrp112.c
> gcc/testsuite/gcc.dg/tree-ssa/vrp112.c
> index e69de29..fdc6711 100644
> --- gcc/testsuite/gcc.dg/tree-ssa/vrp112.c
> +++ gcc/testsuite/gcc.dg/tree-ssa/vrp112.c
> @@ -0,0 +1,31 @@
> +/* PR tree-optimization/78819 */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +__attribute__((noinline, noclone)) void
> +foo (int argc)
> +{
> + if (argc <= 0 || argc > 3)
> +return;
> +
> + switch (argc)
> +{
> +case 1:
> +case 3:
> + if (argc != 3)
> + __builtin_abort ();
> + break;
> +case 2:
> + asm ("");
> + break;
> +default:
> + __builtin_abort ();
> +}
> +}
> +
> +int
> +main (void)
> +{
> + foo (3);
> + return 0;
> +}
> diff --git gcc/tree-vrp.c gcc/tree-vrp.c
> index 97e9953..405469a 100644
> --- gcc/tree-vrp.c
> +++ gcc/tree-vrp.c
> @@ -6051,10 +6051,17 @@ find_switch_asserts (basic_block bb, gswitch *last)
>/* Now register along the default label assertions that correspond to the
> anti-range of each label. */
>int insertion_limit = PARAM_VALUE (PARAM_MAX_VRP_SWITCH_ASSERTIONS);
> + if (insertion_limit == 0)
> +return;
> +
> + /* We can't do this if the default case shares a label with another case.
> */
> + tree default_cl = gimple_switch_default_label (last);
>for (idx = 1; idx < n; idx++)
> {
>tree min, max;
>tree cl = gimple_switch_label (last, idx);
> + if (CASE_LABEL (cl) == CASE_LABEL (default_cl))
> + break;
It's conservative to break here but don't you actually want to continue instead?
>min = CASE_LOW (cl);
>max = CASE_HIGH (cl);
> @@ -6065,6 +6072,8 @@ find_switch_asserts (basic_block bb, gswitch *last)
> {
> tree next_min, next_max;
> tree next_cl = gimple_switch_label (last, idx);
> + if (CASE_LABEL (next_cl) == CASE_LABEL (default_cl))
> + break;
here the break is of course correct.
Ok with that change.
Thanks,
Richard.
> next_min = CASE_LOW (next_cl);
> next_max = CASE_HIGH (next_cl);
>
> Marek
