Re: Request for code review - (ZEE patch : Redundant Zero extension elimination)

[Diego Novillo <dnovillo at google dot com>]

On 5/3/10 18:56 , Sriraman Tallam wrote:

> +
> +shr $0x5, %edx
> +mov %edx, %edx --> Useless zero-extend.
> +
> +How to turn on ?
> +----------------
> +-fzee -O2
> +
> +*/

Please indent this whole comment two spaces.

> +
> +/* Check to see if this zero-extend matches a pattern
> +   that could be eliminated.  This is called via
> +   for_each_rtx in function find_and_remove_ze. */

Two spaces after '.'

> +   Special Cases :
> +   If the expression is a constant or another zero_extend directly
> +   assign it to the 64-bit register.
> +*/

Move closing comment to previous line.

> +              val = INTVAL (orig_src);
> +
> +              /* Zero-extending a negative 32-bit integer into 64-bits makes
> +                 it a positive integer.  Convert the given negative integer
> +                 into the appropriate postive integer when zero-extended. */

s/postive/positive/

> +
> +              val = 0xffffffff + val + 1;
> +              gcc_assert (val > 0);

This will fail on hosts with ints different than the target.  Don't you
want to do this arithmetic in the target mode here?  This would not be
much of a problem for x86, but it may be in a cross compiler.


> +/* Treat if_then_else insns, where the operands of both branches
> +   are registers, as copies. For instance,
> +   Original :
> +   (set (reg:SI a) (if_then_else (cond) (reg:SI b) (reg:SI c)))
> +   Transformed :
> +   (set (reg:DI a) (if_then_else (cond) (reg:DI b) (reg:DI c))) */
> +
> +static bool
> +transform_ifelse (rtx def_insn)

DEF_INSN needs documentation.  I've marked a few functions that have a
similar issue.  In general, you need to reference all the argument names
when describing what the function does.  All the function arguments
should be capitalized.

> +/* Function to get all the immediate definitions of an instruction.
> +   The reaching definitions are desired for which_reg used in
> +   curr_insn.  This function returns 0 if there was an error getting
> +   a definition.  Upon success, this function returns the number of
> +   definitions. */
> +
> +static int
> +get_defs (rtx curr_insn, rtx which_reg, VEC (rtx,heap) **dest)

CURR_INSN, WHICH_REG and DEST need documentation.

> +
> +  while (def_chain)
> +    {
> +      /* Problem getting some definition for this instruction */
> +

Finish comments with '.  */'

> +
> +/* rtx function to check if this SET insn is a conditional copy insn :
> +   (set (reg:SI a ) (IF_THEN_ELSE (cond) (reg:SI b) (reg:SI c)))
> +   Called from is_insn_cond_copy */
> +
> +static int
> +is_this_a_cmove (rtx expr, void *data)

EXPR and DATA need documentation.

> +
> +  /* Now go through the conditional copies vector and try to merge all
> +     the copies in this vector.*/

Two spaces after '.'.  It occurs in a few other places.

> +}
> +
> +/* Goes through the instruction stream looking for zero-extends.  If the zero
> +   extension instruction has atleast one def it adds it to a list of possible
> +   candidates for deletion.  It returns the list of candidates. */
> +static VEC (rtx,heap)*
> +find_removable_zero_extends (void)

Blank line after comment.

> +}
> +
> +static unsigned int
> +find_and_remove_ze (void)
> +{

Needs comment.

> +{
> + {
> +  RTL_PASS,
> +  "zee",                                /* name */
> +  gate_handle_zee,                      /* gate */
> +  rest_of_handle_zee,                   /* execute */

I never did like this RTL convention of naming passes 'rest_of_'.  But
that's fine :)

>  
> +fzee
> +Common Report Var(flag_zee) Init(0)
> +Eliminate redundant zero extensions on targets that supports implicit extensions.

s/supports/support/

What happens if the user specifies -fzee on a target that does not
support implicit extensions?  The gating function is not checking for that.

The patch needs a ChangeLog entry.  Other than that, it looks OK for commit.


Diego.