Re: [TUPLES][PATCH] Tuplifying tree-ssa-loop-unswitch.c

[Zdenek Dvorak <rakdver at kam dot mff dot cuni dot cz>]

Hi,

> 2008/6/15 Zdenek Dvorak <rakdver@kam.mff.cuni.cz>:
> 
> > I think it would be cleaner to make loop_version interpret its argument
> > as gimple, if we are going to do this:
> 
> I keep this interface between loop_version and
> tree-ssa-loop-unswitch.c intact due to lack of time. I can look at
> loop_version and see if we can do this easily.
> 
> >> +static gimple true_gimple_cond;
> >> +static gimple false_gimple_cond;
> >
> > However, you should consider keeping the code as it was originally, with
> > the condition represented as a tree (which seems to actually simplify
> > the code).  I don't really have a strong preference on this, choose
> > whichever way you prefer.
> 
> This is done on purpose. I tried to avoid building temporary condition
> trees from the flattened condition trees stored inside GIMPLE_COND
> statements, doing to so add times and storage cost.

since you build the trees anyway to pass them to loop_version, this
does not seem to be a strong reason (also, this is not a hot code,
so saving one tree in exchange for complicating the code is not
necessarily a good deal).

> >> +  /* Reconstruct COND_EXPR from gimple.  */
> >> +  if (gimple_cond_code (cond) == NE_EXPR
> >> +      && gimple_cond_rhs (cond) == integer_zero_node)
> >> +    cond_expr = unshare_expr (gimple_cond_lhs (cond));
> >
> > What is the purpose of special-casing this?  The resulting cond_expr
> > will not have boolean type, which might cause problems later,
> 
> Good catch about gimple_cond_lhs (cond) not being boolean type.  Since
> a GIMPLE_COND stmt always stores a comparison operator, we use x != 0
> instead of x even if x a boolean value.  I want to avoid creating a
> temp COND_EXPR node when it is not necessary.  I can make this correct
> by adding a check to ensure gimple_cond_lhs (cond) is of boolean type.
>
> Alternatively, I can also remove this special case to make the code
> simpler at the expense of creating one more tree node than strictly
> necessary.

I would go with the later alternative -- this certainly is not a hot
code,

Zdenek