| Message ID | fc229a23-8b51-5104-326b-3c89c60d9c47@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Strenghten assumption about gswitch statements. | expand |
On 08/27/2018 08:05 AM, Martin Liška wrote: > Hi. > > Now we should not meet a degenerated gswitch statements. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Martin > > gcc/ChangeLog: > > 2018-08-27 Martin Liska <mliska@suse.cz> > > * tree-cfg.c (find_taken_edge_switch_expr): Replace not possible > condition with assert. > * tree-switch-conversion.c (switch_conversion::expand): > Likewise. OK. jeff
On Mon, Aug 27, 2018 at 4:05 PM Martin Liška <mliska@suse.cz> wrote: > > Hi. > > Now we should not meet a degenerated gswitch statements. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? Hum. This relies on us doing CFG cleanup. Do we really want find_taken_edge to ICE when called between CFG construction and the first such call? That is, I think this kind of "verification" belongs in the CFG verifier. Note that there's GIMPLE switches before we have a CFG and those are not sanitized (at least not that I know of). So we cannot really declare single-case switch stmts invalid? Thanks, Richard. > Martin > > gcc/ChangeLog: > > 2018-08-27 Martin Liska <mliska@suse.cz> > > * tree-cfg.c (find_taken_edge_switch_expr): Replace not possible > condition with assert. > * tree-switch-conversion.c (switch_conversion::expand): > Likewise. > --- > gcc/tree-cfg.c | 17 +++++++---------- > gcc/tree-switch-conversion.c | 9 +-------- > 2 files changed, 8 insertions(+), 18 deletions(-) > >
On 08/27/2018 05:21 PM, Richard Biener wrote: > On Mon, Aug 27, 2018 at 4:05 PM Martin Liška <mliska@suse.cz> wrote: >> >> Hi. >> >> Now we should not meet a degenerated gswitch statements. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? > > Hum. This relies on us doing CFG cleanup. Do we really want > find_taken_edge to ICE when called between CFG construction > and the first such call? That is, I think this kind of "verification" > belongs in the CFG verifier. Note that there's GIMPLE switches > before we have a CFG and those are not sanitized (at least > not that I know of). So we cannot really declare single-case > switch stmts invalid? Thanks for explanation. In case of switch_conversion::expand, may I apply patch for that? It should not see a degenerated switch, or do I miss something? Martin > > Thanks, > Richard. > >> Martin >> >> gcc/ChangeLog: >> >> 2018-08-27 Martin Liska <mliska@suse.cz> >> >> * tree-cfg.c (find_taken_edge_switch_expr): Replace not possible >> condition with assert. >> * tree-switch-conversion.c (switch_conversion::expand): >> Likewise. >> --- >> gcc/tree-cfg.c | 17 +++++++---------- >> gcc/tree-switch-conversion.c | 9 +-------- >> 2 files changed, 8 insertions(+), 18 deletions(-) >> >>
On Tue, Aug 28, 2018 at 9:11 AM Martin Liška <mliska@suse.cz> wrote: > > On 08/27/2018 05:21 PM, Richard Biener wrote: > > On Mon, Aug 27, 2018 at 4:05 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> Hi. > >> > >> Now we should not meet a degenerated gswitch statements. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> Ready to be installed? > > > > Hum. This relies on us doing CFG cleanup. Do we really want > > find_taken_edge to ICE when called between CFG construction > > and the first such call? That is, I think this kind of "verification" > > belongs in the CFG verifier. Note that there's GIMPLE switches > > before we have a CFG and those are not sanitized (at least > > not that I know of). So we cannot really declare single-case > > switch stmts invalid? > > Thanks for explanation. In case of switch_conversion::expand, may I apply > patch for that? It should not see a degenerated switch, or do I miss > something? Hopefully not. So that part is OK. Richard. > Martin > > > > > Thanks, > > Richard. > > > >> Martin > >> > >> gcc/ChangeLog: > >> > >> 2018-08-27 Martin Liska <mliska@suse.cz> > >> > >> * tree-cfg.c (find_taken_edge_switch_expr): Replace not possible > >> condition with assert. > >> * tree-switch-conversion.c (switch_conversion::expand): > >> Likewise. > >> --- > >> gcc/tree-cfg.c | 17 +++++++---------- > >> gcc/tree-switch-conversion.c | 9 +-------- > >> 2 files changed, 8 insertions(+), 18 deletions(-) > >> > >> >
On 8/28/18 12:10 PM, Richard Biener wrote: > On Tue, Aug 28, 2018 at 9:11 AM Martin Liška <mliska@suse.cz> wrote: >> >> On 08/27/2018 05:21 PM, Richard Biener wrote: >>> On Mon, Aug 27, 2018 at 4:05 PM Martin Liška <mliska@suse.cz> wrote: >>>> >>>> Hi. >>>> >>>> Now we should not meet a degenerated gswitch statements. >>>> >>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>>> >>>> Ready to be installed? >>> >>> Hum. This relies on us doing CFG cleanup. Do we really want >>> find_taken_edge to ICE when called between CFG construction >>> and the first such call? That is, I think this kind of "verification" >>> belongs in the CFG verifier. Note that there's GIMPLE switches >>> before we have a CFG and those are not sanitized (at least >>> not that I know of). So we cannot really declare single-case >>> switch stmts invalid? >> >> Thanks for explanation. In case of switch_conversion::expand, may I apply >> patch for that? It should not see a degenerated switch, or do I miss >> something? > > Hopefully not. I missed as it caused PR87686. Thus I'm reverting that. Patch is tested on x86_64-linux-gnu. Martin > > So that part is OK. > > Richard. > >> Martin >> >>> >>> Thanks, >>> Richard. >>> >>>> Martin >>>> >>>> gcc/ChangeLog: >>>> >>>> 2018-08-27 Martin Liska <mliska@suse.cz> >>>> >>>> * tree-cfg.c (find_taken_edge_switch_expr): Replace not possible >>>> condition with assert. >>>> * tree-switch-conversion.c (switch_conversion::expand): >>>> Likewise. >>>> --- >>>> gcc/tree-cfg.c | 17 +++++++---------- >>>> gcc/tree-switch-conversion.c | 9 +-------- >>>> 2 files changed, 8 insertions(+), 18 deletions(-) >>>> >>>> >> From 1a1f856006be42bb2f94d9df7bad314651fa8e8c Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Mon, 22 Oct 2018 13:13:08 +0200 Subject: [PATCH] Revert r263947. gcc/ChangeLog: 2018-10-22 Martin Liska <mliska@suse.cz> Revert 2018-08-29 Martin Liska <mliska@suse.cz> * tree-switch-conversion.c (switch_conversion::expand): Strenghten assumption about gswitch statements. gcc/testsuite/ChangeLog: 2018-10-22 Martin Liska <mliska@suse.cz> * g++.dg/tree-ssa/pr87686.C: New test. --- gcc/testsuite/g++.dg/tree-ssa/pr87686.C | 21 +++++++++++++++++++++ gcc/tree-switch-conversion.c | 9 ++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr87686.C diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr87686.C b/gcc/testsuite/g++.dg/tree-ssa/pr87686.C new file mode 100644 index 00000000000..65160a79855 --- /dev/null +++ b/gcc/testsuite/g++.dg/tree-ssa/pr87686.C @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +class a { +public: + enum b { c, g, d, e } f; + a(b h) : f(h) {} + a i() { + switch (f) { + case d: + return c; + case e: + return g; + } + } /* { dg-warning "control reaches end of non-void function" } */ +}; +struct k { + a j; + k l() { j.i(); } /* { dg-warning "no return statement in function returning non-void" } */ +}; +void m(k h) { h.l(); } diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c index 64169a6cd3d..ac2aa585257 100644 --- a/gcc/tree-switch-conversion.c +++ b/gcc/tree-switch-conversion.c @@ -913,7 +913,14 @@ switch_conversion::expand (gswitch *swtch) /* Group case labels so that we get the right results from the heuristics that decide on the code generation approach for this switch. */ m_cfg_altered |= group_case_labels_stmt (swtch); - gcc_assert (gimple_switch_num_labels (swtch) >= 2); + + /* If this switch is now a degenerate case with only a default label, + there is nothing left for us to do. */ + if (gimple_switch_num_labels (swtch) < 2) + { + m_reason = "switch is a degenerate case"; + return; + } collect (swtch);
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index cf12cb1f391..338967f39f7 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -2443,17 +2443,14 @@ find_taken_edge_switch_expr (const gswitch *switch_stmt, tree val) edge e; tree taken_case; - if (gimple_switch_num_labels (switch_stmt) == 1) - taken_case = gimple_switch_default_label (switch_stmt); + gcc_assert (gimple_switch_num_labels (switch_stmt) >= 2); + if (val == NULL_TREE) + val = gimple_switch_index (switch_stmt); + if (TREE_CODE (val) != INTEGER_CST) + return NULL; else - { - if (val == NULL_TREE) - val = gimple_switch_index (switch_stmt); - if (TREE_CODE (val) != INTEGER_CST) - return NULL; - else - taken_case = find_case_label_for_value (switch_stmt, val); - } + taken_case = find_case_label_for_value (switch_stmt, val); + dest_bb = label_to_block (cfun, CASE_LABEL (taken_case)); e = find_edge (gimple_bb (switch_stmt), dest_bb); diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c index a31ff94b895..7e4f34c71f8 100644 --- a/gcc/tree-switch-conversion.c +++ b/gcc/tree-switch-conversion.c @@ -913,14 +913,7 @@ switch_conversion::expand (gswitch *swtch) /* Group case labels so that we get the right results from the heuristics that decide on the code generation approach for this switch. */ m_cfg_altered |= group_case_labels_stmt (swtch); - - /* If this switch is now a degenerate case with only a default label, - there is nothing left for us to do. */ - if (gimple_switch_num_labels (swtch) < 2) - { - m_reason = "switch is a degenerate case"; - return; - } + gcc_assert (gimple_switch_num_labels (swtch) >= 2); collect (swtch);