Fix checking of gimple types

On Thu, 2013-11-21 at 18:03 -0500, Andrew MacLeod wrote:
> On 11/21/2013 05:42 PM, Jakub Jelinek wrote:
> > On Thu, Nov 21, 2013 at 03:24:55PM -0700, Jeff Law wrote:
> >> On 11/21/13 15:19, Jakub Jelinek wrote:
> >>> On Mon, Nov 18, 2013 at 03:25:52PM -0500, David Malcolm wrote:
> >>>>> So is there some reason the GIMPLE_CHECK was left in here rather than
> >>>>> doing the downcasting?  This happens in other places.
> >>> Note that the changes removed tons of checks that IMHO were desirable.
> >>> The as_a that replaced those checks e.g. allows 3 different gimple codes,
> >>> while previously only one was allowed, this is both more expensive for
> >>> --enable-checking=yes, and allows one to use inline wrappers e.g.
> >>> gimple_omp_parallel_something on GIMPLE_OMP_TASK etc.
> >> Can you give a couple examples, please?
> > I mean e.g.
> > gimple_omp_parallel_{,set_}{clauses,child_fn,data_arg}{,_ptr}
> > gimple_omp_taskreg_{,set_}{clauses,child_fn,data_arg}{,_ptr}
> > gimple_omp_target_{,set_}{clauses,child_fn,data_arg}{,_ptr}
> > gimple_omp_teams_{,set_}clauses{,_ptr}
> > gimple_omp_return_{,set_}lhs{,_ptr}
> > gimple_omp_atomic_store_{,set_}val{,_ptr}
> > gimple_resx_{,set_}region
> > gimple_eh_dispatch_{,set_}region
> >
> > 	Jakub
> Why does  is_a_helper <gimple_statement_omp_parallel>::test allow 
> anything other than a GIMPLE_OMP_PARALLEL..?  That seems wrong to me. 
> should just be the one check.
> 
> gimple_omp_taskreg and other routines "sharing" that helper should have 
> their own helper and only check the one code.. thats is whole point to 
> remain at least codegen neutral in these cases and provide correct 
> checking.   The fact that they may happen to share the same underlying 
> structure is irrelevant.
> 
> I also think this is wrong.

Mea culpa.  Unfortunately I made a conceptual error during the
conversion (the worst kind of error).  I misunderstood the relationships
between the various OMP statements: there (mostly) aren't any, but the
sharing of structures for layout made me think there were.

Attached is a patch, successfully bootstrapped&regtested on
x86_64-unknown-linux-gnu, which I believe reinstates the checked-build
behaviors from before r205034 (and that the unchecked-build behaviors
were not affected by that commit and likewise are not by this patch).

As I understand it, there are almost no "is-a" relationships between the
various omp statement types, some of them just happen to share layouts.
The exception is that the various gimple_omp_taskreg_* accessors accept
either codes GIMPLE_OMP_PARALLEL or GIMPLE_OMP_TASK and so an
"omp_taskreg" is a concept, of which OMP_PARALLEL and OMP_TASK have an
is-a relationship.

Based on this, I've reworked the is_a_helper functions, eliminating
almost all of the ones that accepted multiple codes.  The only ones that
remain accepting multiple codes are those for:

  * gimple_statement_with_ops, which uses gimple_has_ops (gs), and for
  * gimple_statement_with_memory_ops, which uses gimple_has_mem_ops
(gs), plus
  * a new class gimple_statement_omp_taskreg, which expresses the
"either GIMPLE_OMP_PARALLEL or GIMPLE_OMP_TASK" condition, and becomes a
parent struct for those.

I introduced some new structs to express the pre-existing layouts for
GSS codes, and to distinguish them from structs that imply a specific
GIMPLE_ code.
For example,
  gimple_statement_omp_atomic_store
now requires that the code be GIMPLE_OMP_ATOMIC_STORE, but it was also
the name of a layout, GSS_OMP_ATOMIC_STORE.  So I renamed these purely
layout classes, so that there is now a
  gimple_statement_omp_atomic_store_layout
class for the corresponding GSS value, which I renamed to
  GSS_OMP_ATOMIC_STORE_LAYOUT
to make clear that this is just a layout: that although
GIMPLE_OMP_RETURN happens to share the data layout of
GIMPLE_OMP_ATOMIC_STORE, they are not otherwise related - they now both
inherit from the "_layout" class.

I'm not a fan of these "_layout" names, but I'm not sure what better to
call them. Perhaps:
   GSS_OMP_PARALLEL_LAYOUT     -> GSS_OMP_WITH_CLAUSES_CHILD_FN_DATA_ARG
   GSS_OMP_SINGLE_LAYOUT       -> GSS_OMP_WITH_CLAUSES
   GSS_OMP_ATOMIC_STORE_LAYOUT -> GSS_OMP_WITHOUT_SEQ_WITH_VAL
with analogous names for the corresponding structs.

I added GTY tags for every class in the hierarchy, not just those that
introduce a new layout, since gengtype only recognizes inheritance when
supplied a "tag" option.  This leads to the GSS values appearing
multiple times within the class hierarchy, which required a patch to
gengtype, to prevent duplicate case labels in the generated switch
statement.

I believe that this structure correctly reinstates the exact behavior
from before the inheritance patch for the checked build, and that either
way, the behavior in the unchecked build is the same.

Another approach to this would be to entirely eliminate these shared
layout types, going purely with the conceptual is-a relationships
between the types, say, by replacing the gengtype switch on GSS_ value
with a switch on GIMPLE_ code.  Given that this might affect the speed
of GC (bigger switch statements), I went with the relatively more
conservative change.

Successfully bootstrapped&regtested on x86_64-unknown-linux-gnu.

OK for trunk?

Sorry again for breaking this.

Here's an ASCII art inheritance diagram, showing the relationships
between structs, GSS_ values (layouts) and GIMPLE_ values (codes) - as I
see them.  If I'm still misunderstanding something, sorry, please let me
know.  Should this diagram live in gimple.h?
+- gimple_statement_base
     |    layout: GSS_BASE.  4 codes use this layout.
     |
     + gimple_statement_with_ops_base
     |   |    (no GSS layout)
     |   |
     |   + gimple_statement_with_ops
     |   |        layout: GSS_WITH_OPS.   5 codes use this layout.
     |   |
     |   + gimple_statement_with_memory_ops_base
     |       |    layout: GSS_WITH_MEM_OPS_BASE
     |       |
     |       + gimple_statement_with_memory_ops
     |       |        layout: GSS_WITH_MEM_OPS.
     |       |        Used by codes GIMPLE_ASSIGN and GIMPLE_RETURN.
     |       |
     |       + gimple_statement_call
     |       |        layout: GSS_CALL, code: GIMPLE_CALL
     |       |
     |       + gimple_statement_asm
     |       |        layout: GSS_ASM, code: GIMPLE_ASM
     |       |
     |       + gimple_statement_transaction
     |                layout: GSS_TRANSACTION, code: GIMPLE_TRANSACTION
     |
     + gimple_statement_omp
     |   |    layout: GSS_OMP
     |   |
     |   + gimple_statement_omp_critical
     |   |        layout: GSS_OMP_CRITICAL, code: GIMPLE_OMP_CRITICAL
     |   |
     |   + gimple_statement_omp_for
     |   |        layout: GSS_OMP_FOR, code: GIMPLE_OMP_FOR
     |   |
     |   + gimple_statement_omp_parallel_layout
     |   |   |    layout: GSS_OMP_PARALLEL_LAYOUT
     |   |   |
     |   |   + gimple_statement_omp_taskreg
     |   |   |   |
     |   |   |   + gimple_statement_omp_parallel
     |   |   |   |        code: GIMPLE_OMP_PARALLEL
     |   |   |   |
     |   |   |   + gimple_statement_omp_task
     |   |   |            code: GIMPLE_OMP_TASK
     |   |   |
     |   |   + gimple_statement_omp_target
     |   |            code: GIMPLE_OMP_TARGET
     |   |
     |   + gimple_statement_omp_sections
     |   |        layout: GSS_OMP_SECTIONS, code: GIMPLE_OMP_SECTIONS
     |   |
     |   + gimple_statement_omp_single_layout
     |       |    layout: GSS_OMP_SINGLE_LAYOUT
     |       |
     |       + gimple_statement_omp_single
     |       |        code: GIMPLE_OMP_SINGLE
     |       |
     |       + gimple_statement_omp_teams
     |                code: GIMPLE_OMP_TEAMS
     |
     + gimple_statement_bind
     |        layout: GSS_BIND, code: GIMPLE_BIND
     |
     + gimple_statement_catch
     |        layout: GSS_CATCH, code: GIMPLE_CATCH
     |
     + gimple_statement_eh_filter
     |        layout: GSS_EH_FILTER, code: GIMPLE_EH_FILTER
     |
     + gimple_statement_eh_else
     |        layout: GSS_EH_ELSE, code: GIMPLE_EH_ELSE
     |
     + gimple_statement_eh_mnt
     |        layout: GSS_EH_MNT, code: GIMPLE_EH_MUST_NOT_THROW
     |
     + gimple_statement_phi
     |        layout: GSS_PHI, code: GIMPLE_PHI
     |
     + gimple_statement_eh_ctrl
     |   |    layout: GSS_EH_CTRL
     |   |
     |   + gimple_statement_resx
     |   |        code: GIMPLE_RESX
     |   |
     |   + gimple_statement_eh_dispatch
     |            code: GIMPLE_EH_DISPATCH
     |
     + gimple_statement_try
     |        layout: GSS_TRY, code: GIMPLE_TRY
     |
     + gimple_statement_wce
     |        layout: GSS_WCE, code: GIMPLE_WITH_CLEANUP_EXPR
     |
     + gimple_statement_omp_continue
     |        layout: GSS_OMP_CONTINUE, code: GIMPLE_OMP_CONTINUE
     |
     + gimple_statement_omp_atomic_load
     |        layout: GSS_OMP_ATOMIC_LOAD, code: GIMPLE_OMP_ATOMIC_LOAD
     |
     + gimple_statement_omp_atomic_store_layout
         |    layout: GSS_OMP_ATOMIC_STORE_LAYOUT,
         |    code: GIMPLE_OMP_ATOMIC_STORE
         |
         + gimple_statement_omp_atomic_store
         |        code: GIMPLE_OMP_ATOMIC_STORE
         |
         + gimple_statement_omp_return
                  code: GIMPLE_OMP_RETURN

Fix checking of gimple types

Commit Message

Comments

Patch