Re: [PATCH, middle-end] Switch initializations conversion

[Martin Jambor <jamborm at matfyz dot cz>]

Hi,

thank you very much for the review. Sorry for the late reply, I was
sick last week and it is taking me a lot of time to catch up. 

I hope to post a new patch this week.

On Thu, Oct 25, 2007 at 04:53:35PM -0700, Richard Henderson wrote:
> On Tue, Sep 11, 2007 at 02:23:00PM +0200, Martin Jambor wrote:
> > +There are further constraints. Specifically, the range of values across all
> > +case labels must be smaller than 0x2000 and must not be bigger than eight times
> 
> Comments are out of date wrt the PARAM values.

I'll fix that.

> Why exactly is there a max range on the case labels at all?  Shouldn't
> this in fact be tied to the jump table density instead?  You can pretty
> much be sure that a size N jump table plus N assignment and jump statements
> is going to be bigger than an array of size N containing just the data.

I guess you're right, I'll remove it.
 
> Currently the choice whether to use a jump table or a set of branches --
> effectively the density choice you want -- is encoded directly in
> expand_case, but there's no reason you couldn't pull that out so that you
> could reuse it.

Will look into it.

> > +/* The following three functions evaluate "equal to" "less then," and "less
> > +   than or equal to" (receptively) of two integers represented by
> > +   INTEGER_CSTs.  */
> 
> Err, is tree_int_cst_lt, tree_int_cst_equal, or tree_int_cst_compare
> not good enough for you for some reason?  That said...

No, they're great. I just didn't find them and people I asked pointed
me to other directions.

> This whole block of code is redundant.  By the time we get to the
> optimizers, the gimplifier has sorted the case label array by
> CASE_LOW, and the default label is forced to exist, and it is
> always the last entry in the case label array.

OK, I didn't  know and thought I'd rather be  careful, I'll change the
code accordingly.

> > +/* check_phi_edges() traverses all predecessors of final_bb in the cfg and uses
> > +   rel_bbs to ensure all incoming edges come from the switch.  It returns true
> > +   in this case, false otherwise.  */
> > +static bool
> > +check_phi_edges (void)
> 
> "phi_edges" is less accurate a name than "incoming_edges".

OK

> You could probably avoid this check, with a little work.  Consider:
> 
> 	x0 = ...
> 	some code that conditionally branches to done;
>   BBSWITCH:
> 	switch (i)
> 	{
> 	case 1:
> 	  x1 = 1;
> 	  break;
> 	case 2:
> 	  x2 = 2;
> 	  break;
> 	default:
> 	  x3 = 3;
> 	  break;
> 	}
>   BBFINAL:
>       done:
>       x4 = phi(x0, x1, x2, x3)
> 
> Here we've an extra edge incoming to the final_bb.  The transformation
> could be, instead,
> 
>   BBSWITCH:
>     tmp = i - 1;
>     x5 = 3;
>     if (tmp >= 2) goto BBFINAL;
>     x6 = cswitch[tmp];
>   BBFINAL:
>     x4 = phi(x0, x5, x6)

Makes sense, I'll try.

> where we preserve the phi arguments that came from other blocks outside
> the switch and replace the others with new ssa names containing the default
> and the table-read values.  This also means insert the new code in the
> switch block rather than in the final block as you do now.
> 
> > +  sprintf (name_buf, "CSWTCH%02u", name_num++);
> 
> This name is in the user's namespace.  Use create_tmp_var_name.

OK
 
> > +  fetch = build4 (ARRAY_REF, TREE_TYPE (dflt), decl, index_expr,
> > +		  range_min, NULL_TREE); 
> 
> I don't see anywhere that this index_expr is changed from "i"
> to "i - min"?

The operand 2 (range_min) takes care of that.  Otherwise the
transformation wouldn't have worked.
 
> > +      ref = branch_list; 
> 
> For what reason do you create this branch list, rather than just
> using the PHI nodes as they stand?

Well, given that switch cases are always sorted, I'll give it a try.

> > +	      index = fold_build2 (MINUS_EXPR, index_type, index, 
> > +				  integer_one_node);
> 
> Arithmetic on items known to be CSTs should go through int_const_binop.

Another thing I have been looking for quite some time. Thanks!

Martin