Re: [PATCH, tree-dce]: DCE enh to handle dead calls (not pure)

["Steven Bosscher" <stevenb dot gcc at gmail dot com>]

xf. http://gcc.gnu.org/ml/gcc-patches/2008-05/msg01011.html


Hi David,

It sounds like an interesting transformation -- but could you show an
example (in pseudo code, or a gimple dump) of what this transformation
does, exactly?

There are some issues with your patch. First of all, why all the
random whitespace changes?!  There are a lot of them at the end of
your patch in the hunks for tree-ssa-dce.c, and some places where you
turned tabs into spaces for some reason.  Please clean up your patch
(and your source tree) next time before submitting a patch.
Also, please fix all the lines longer than 80 characters.

Second, the patch assumes there is such a thing as errno, and the
builtin functions behave POSIX-like.  Is this guaranteed?  I think not
(e.g. -ffreestanding, -fno-math-errno?).

Third, at least for my taste you have *way* too many gcc_asserts.
They are not free, you know, and you are asserting trivial things and
things that you have already asserted before.  For example, in the
gen_conditions_for* functions you re-assert everything you already
asserted in the check* functions. Please reconsider whether you really
need all of them.  gcc_assert is not free and also doesn't improve the
readability of the code...


Then, the patch:

> ChangeLog:
>
> 2008-05-17 Xinliang David Li   davidxl@google.com
> gcc/tree-ssa-dce.c : conditional dead call elimination
> gcc/opts.c         : enable the optimization at >=O2
> gcc/common.opt      : new flag for control the optimization
> gcc/doc/invoke.texi : documentation change
> gcc/testsuite/gcc.dg/cdce1.c : new test case
> gcc/testsuite/gcc.dg/cdce2.c : new test case
> MAINTAINERS  : Add myself (write after approval)

That doesn't even look like a GCC ChangeLog entry.  Please look at
ChangeLog (the file) to see what it should look like.


> @@ -5157,6 +5157,7 @@ compilation time.
>  -ftree-ch @gol
>  -ftree-copyrename @gol
>  -ftree-dce @gol
> +-ftree-builtin-dce @gol
> >  -ftree-dominator-opts @gol
>  -ftree-dse @gol
>  -ftree-fre @gol

... and ...

> +@item -ftree-builtin-dce
> +@opindex ftree-builtin-dce
> +Perform conditional dead code elimination (DCE) on builtin calls that
> +may set errno but are otherwise side-effect free.  This flag is enabled by
> +default at @option{-O} and higher.

So what is it: At -O and higher as indicated in this invoke.texi hunk,
or at -O2 and higher, like you say in your changelog?
Further, I suggest you change this to say something about "DCE for
calls to builtin functions" instead of "DCE on builtin calls". The
calls are not builtin, after all ;-)  This is user documentation, and
I think Joe Average User will find this entry confusing...


> +  if (dump_file && (dump_flags & TDF_DETAILS))
> +   {
> +     location_t loc;
> +     loc = EXPR_LOCATION (bi_call);
> +     inform (
> +         "%Hfunction call is shrink-wrapped into error conditions.",
> +         &loc);
> +   }

You don't want to inform here.  You want to write out something to the
dump_file.  This also means your test cases should not be using
dg-message.  They should scan in the DCE dump file instead.

> +
> +static bool
> +check_pow (tree pow_call)
> +{

Missing comment before the function.


> +     def = SSA_NAME_DEF_STMT (base);
> +     if (TREE_CODE (def) != GIMPLE_MODIFY_STMT)
> +       return false;
> +
> +     nm = GIMPLE_STMT_OPERAND (def,0);
> +     gcc_assert (TREE_CODE (nm) == SSA_NAME);
> +     if (nm != base)
> +       return false;

The second block here looks redundant to me.  If operand 0 of the the
SSA_NAME_DEF_STMT for base is not base, something is seriously wrong.
So nm == base always, you can rely on this, and the "nm = ..." block
should go away.

In fact, I don't understand the entire block after "+  else if (bc ==
SSA_NAME)".  What are you trying to check for here?  What does
INT_TYPE_SIZE matter for this transformation?  Since you add virtually
no comment in your functions, it is hard to tell...

(Hmm, I see you explain it a bit further down in a different function.
 Might I suggest grouping the check* and gen_conditions_for* function
by buitlin? I think that would make it easier to follow what is going
on.)

> +
> +static bool
> +check_log (tree log_call)
> +{

Missing comment before the function.


> +/*  A helper function to determine if a builtin function
Should have only one space before "A".


> +  switch (fnc)
> +   {
> +     CASE_FLT_FN (BUILT_IN_POW):
> +         if (check_pow (call))
> +           is_unnecessary_except_errno = true;
> +     break;
> +
> +     CASE_FLT_FN (BUILT_IN_LOG):
> +         if (check_log (call))
> +           is_unnecessary_except_errno = true;
> +     break;
> +     default :
> +       is_unnecessary_except_errno = false;
> +     break;
> +   }

Wrong indentation, for the whole switch block.


> +  if (!is_unnecessary_except_errno)
> +    return false;
> +
> +  return is_unnecessary_except_errno;
> +}

This is one for the daily-wtf :-)  Remove the if, please.


> +/*  The method to generate error condition guard code for log(x)
Should have only one space before "The". Same for the leading comments
before some other new functions.


> +/*  Method to generate conditional statements for guarding condionally
> +    dead calls to pow. One or more statements can be generated for
> +    each logical condition. Statement groups of different conditions
> +    are separated by a NULL tree.  */
> +static void
> +gen_conditions_for_pow (tree pow_call, enum built_in_function fnc,
> +                        VEC (tree, heap)* conds, unsigned * nconds)
> +{

The comment before this function is not really complete, is it?
Separated by a NULL where?  In VEC conds I suppose?  But you did not
say anything about the arguments in the comment (which you should have
done) so it's hard to tell without reading the function.  Sort of
defeats the purpose of documenting an interface ;-)


> +#define ERR_PROB 0.01
Add a comment before this.  Randomly appearing defines are confusing.


> +  /* Now it is time to insert the first condtional expression
> +     into bi_call_bb and split this bb so that bi_call is
> +     shrink-wrapped*/
Not a GCC-style comment.

> +  /* now the label*/
Not a GCC-style comment.

> +  /* code generation for the rest of the conditions */
Not a GCC-style comment.

> +  /* code generation for the rest of the conditions */
Not a GCC-style comment.

> +  for (; nconds > 0; )
And the lord cried "while"!


> +     nconds --;
> +     ci ++;
Not GCC code style.

> +     bi_call_in_edge->probability = REG_BR_PROB_BASE*ERR_PROB;
Not GCC code style (spaces around "*").



The idea is of this patch is really nice. But please try to avoid
submitting patches with so many silly little issues!

Gr.
Steven