[PATCH] support to flip modes in sh mode-switching

[Steven Bosscher]

On Friday 15 December 2006 17:26, Christian BRUEL wrote:
> The problem was described by Joern Rennecke in bugzilla #29439 (part 3).

I'm afraid it was not, because that bug is a VRP problem.
Got the correct bug number?

> 1) In order to add this mode information on edges, I added an 'aux2' in
> the edge_def struct. The alternatives could have been:
>   - use the already existing 'aux' field. But it is broken by
> pre_edge_lcm and the cost of saving restoring it might be high (vector
> or size #of edges * # of entities).
>   - have a local edge vector. but that seems redundant.
>
> there is a also ann additional memory cost for other targets.

And a rather high one, too.

You're proposing to have an aux2 field on _every_ edge for _every_
target, when we're trying to reduce memory consumption instead of
increasing it again...

I think this aux2 field is simply unacceptable.


> 2) the EMIT_MODE_SET macro was extended with a new parameter that is the
> actual mode, when known, or -1.
> after rethought, I would prefer to have a new macro, EMIT_MODE_SWITCH
> that seems closer to the semantic. This macro could be use to #undef
> code, such as the 'aux2' field in edge_def.

You can't #undef that, edge_def is in GC space, and gengtype does
not grok #ifdefs in structures.


> the other local changes to mode-switching.c are
> - avin is now a parameter to pre_edge_lcm

I really dislike this, because you allocate and then free the bitmap
in most of the passes without ever looking at it.  I see you need it
in mode-switching.c but you don't explain why.

(In fact you don't explain at all what this patch does).

You also don't update the comment before lcm.c:pre_edge_lcm.

It seems to me that you could do all of this a lot cleaner if you
factor out the compute_available call out of pre_edge_lcm (and of
its reverse CFG counterpart).  Just compute the avail bitmaps
explicitly in all places and call the LCM routines after that.


> - EMIT_MODE_SET is now called lazily, since the insertion should wait
> for all modes to be processed and value know before emitting the rtl.
>
> validated it on sh4-300 with gcc testsuite.

Bootstrapped on some native target, too?


> +static void
> +init_mode_sets (void)
> +{
> +  struct edge_list* edge_list = create_edge_list ();
> +  int n = NUM_EDGES (edge_list);
> +
> +  edge_mode_inserts = xcalloc (n, sizeof (int));
> +  memset (edge_mode_inserts, -1, n * sizeof (int));
> +}

Why xcalloc something, and then memset it to something else?


> +init_edge_auxes (int n_entities)
> +{
> +  basic_block bb;
> +
> +  FOR_BB_BETWEEN (bb, ENTRY_BLOCK_PTR, EXIT_BLOCK_PTR, next_bb)

You can use FOR_ALL_BB here.  Likewise in free_edge_auxes.


> +	    ((int *)e->aux2)[j] = ((int *)e2->aux2)[j];

I'd very much prefer you introduce a macro somewhere instead of
these explicit casts all over.

> +  set_edge_modes_internal(cfun->cfg->x_entry_block_ptr->next_bb, visited, info, j, no_mode);

Line too long.  Also, why not just ENTRY_BLOCK_PTR->next_bb?

You have no comments before most functions, so it's impossible to
tell what you've tried to implement and review that.  There are
also lots of little coding style issues.

You may want to read http://gcc.gnu.org/codingconventions.html
and http://gcc.gnu.org/contribute.html#patches if you have not
already done so.

Gr.
Steven