Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Martin Liška <mliska at suse dot cz>]

On 09/13/2017 08:41 PM, Jeff Law wrote:
> On 09/13/2017 12:09 PM, Martin Liška wrote:
> > On 09/13/2017 04:22 PM, Jeff Law wrote:
> > > On 09/13/2017 07:42 AM, Martin Liška wrote:
> > > > On 09/13/2017 03:08 PM, Martin Liška wrote:
> > > > > On 09/12/2017 05:21 PM, Jeff Law wrote:
> > > > > > On 09/12/2017 01:43 AM, Martin Liška wrote:
> > > > > > > Hello.
> > > > > > >
> > > > > > > In transition to simple_case_node, I forgot to initialize m_high to m_low if a case
> > > > > > >
> > > > > > > does not have CASE_HIGH.
> > > > > > >
> > > > > > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> > > > > > >
> > > > > > >
> > > > > > > Ready to be installed?
> > > > > > > Martin
> > > > > > >
> > > > > > > gcc/ChangeLog:
> > > > > > >
> > > > > > > 2017-09-11  Martin Liska  <mliska@suse.cz>
> > > > > > >
> > > > > > >      PR middle-end/82154
> > > > > > >      * stmt.c (struct simple_case_node): Assign low to high when
> > > > > > >      high is equal to null.
> > > > > > >
> > > > > > > gcc/testsuite/ChangeLog:
> > > > > > >
> > > > > > > 2017-09-11  Martin Liska  <mliska@suse.cz>
> > > > > > >
> > > > > > >      PR middle-end/82154
> > > > > > >      * g++.dg/torture/pr82154.C: New test.
> > > > > > OK.
> > > > > >
> > > > > > THough I have to wonder if we should unify the HIGH handling -- having
> > > > > > two different conventions is bound to be confusing.
> > > > > >
> > > > > > In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label
> > > > > > refers to a singleton value that is found in CASE_LOW.
> > > > > >
> > > > > > That means we end up doing stuff like this:
> > > > > >
> > > > > >    if (CASE_HIGH (elt))
> > > > > >       maxval = fold_convert (index_type, CASE_HIGH (elt));
> > > > > >     else
> > > > > >       maxval = fold_convert (index_type, CASE_LOW (elt));
> > > > > >
> > > > > >
> > > > > >
> > > > > > You could legitimately argue for changing how this works for tree nodes
> > > > > >
> > > > > > so that there's always a non-null CASE_HIGH.
> > > > >
> > > > > Hi.
> > > > >
> > > > > Agree with you that we have a lot of code that does what you just described.
> > > > >
> > > > > I tent to change IL representation, where CASE_HIGH is always non-null.
> > > > >
> > > > > $ git grep 'CASE_HIGH\>' | wc -l
> > > > > 125
> > > > >
> > > > > Which is reasonable amount of code that should be changed. I'll prepare patch for that.
> > > >
> > > > Hm, there's one question that pops up: Should we compare the values via pointer equality,
> > > >
> > > > or tree_int_cst_eq should be done? The later one can messy the code a bit.
> > > I'd like to say pointer equality, but it's probably safer to use
> > > tree_int_cst_eq.
> > >
> > > jeff
> >
> > Ok, so I'll put it on my TODO list as it will take some time to do the transformation.
> NP.  There's nothing inherently wrong with the code today, it's just a
> cleanup (I hope :-)

Yes.

> > In order to fix the PR, I suggest following patch.
> >
> > Ready for trunk?
> > Martin
> >
> > 0001-Fix-emission-of-exception-dispatch-PR-middle-end-821.patch
> >
> >
> >  From 1cd8987ad3de4a7bd3e71014a0df9ccb3be40277 Mon Sep 17 00:00:00 2001
> > From: marxin <mliska@suse.cz>
> > Date: Mon, 11 Sep 2017 13:34:41 +0200
> > Subject: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).
> >
> > gcc/ChangeLog:
> >
> > 2017-09-11  Martin Liska  <mliska@suse.cz>
> >
> > 	PR middle-end/82154
> > 	* stmt.c (expand_sjlj_dispatch_table): Use CASE_LOW when
> > 	CASE_HIGH is NULL_TREE.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2017-09-11  Martin Liska  <mliska@suse.cz>
> >
> > 	PR middle-end/82154
> > 	* g++.dg/torture/pr82154.C: New test.
> OK.
>
> Jeff

Thanks, I've just installed it.

Martin