This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Simplify some PRE stuff


On Sun, Mar 9, 2008 at 7:13 AM, Richard Guenther <rguenther@suse.de> wrote:
>
>  While munging with PRE I noticed the following.  get_sccvn_value seems
>  overly complicated (and has this strange defstmt = NULL and later assert
>  for it code), so we just simplify it do do what the comment says
>  (I notice the commend in the deleted block does no longer apply as we
>  now lookup the vn without vuses).

Yes, since you changed it to use vn_lookup_or_add, the rest no longer applies.


>  (for unrecognized stmts) adding uses to the sets.  This doesn't make
>  sense to me as they should already be in as we iterate in dominator
>  order and the avail sets get initialized with the dominating definition
>  blocks of these uses.  If valid this iteration needs a comment at least.
>

If you want to remove the adding to AVAIL, that is fine.
You cannot remove the adding to EXP_GEN.
EXP_GEN represents those expressions that appear on the RHS of
statements in a given block.
Even "unrecognized" uses should appear in EXP_GEN.

>  Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for mainline?

You should be aware that I made a lot of the changes you are undoing
after analyzing PRE's behavior on a large number of testcases I can't
submit (or easily minimize). I also used to ensure the number of
eliminations made during a gcc bootstrap didn't change.  You should be
very careful about changing it and expecting okay results just because
it bootstraps and passes the small number of optimization regtests we
have for it.
It is generallly a good idea to add asserts to make sure you are not
changing behavior and bootstrap with those, and for overall behavior,
ensuring the number of eliminations made during a bootstrap doesn't
change.

In any case, this patch is fine except for the adding to EXP_GEN part,
which you shouldn't remove.
(The reason it does not necessarily change anything in most cases is
because I corrected the dataflow equations for ANTIC, so the default
ANTIC set usually includes most of EXP_GEN)

>
>  Thanks,
>  Richard.
>
>  2008-03-09  Richard Guenther  <rguenther@suse.de>
>
>         * tree-ssa-pre.c (get_sccvn_value): Simplify.
>         (compute_avail): Do not add stmt uses to AVAIL_OUT or EXP_GEN.
>
>  Index: tree-ssa-pre.c
>  ===================================================================
>  *** tree-ssa-pre.c      (revision 133040)
>  --- tree-ssa-pre.c      (working copy)
>  *************** get_sccvn_value (tree name)
>  *** 3243,3261 ****
>          it.  */
>         if (!valvh && !is_invariant)
>         {
>  -         tree defstmt = SSA_NAME_DEF_STMT (val);
>  -
>  -         gcc_assert (VN_INFO (val)->valnum == val);
>  -         /* PHI nodes can't have vuses and attempts to iterate over
>  -            their VUSE operands will crash.  */
>  -         if (TREE_CODE (defstmt) == PHI_NODE || IS_EMPTY_STMT (defstmt))
>  -           defstmt = NULL;
>  -         {
>  -           tree defstmt2 = SSA_NAME_DEF_STMT (name);
>  -           if (TREE_CODE (defstmt2) != PHI_NODE &&
>  -               !ZERO_SSA_OPERANDS (defstmt2, SSA_OP_ALL_VIRTUALS))
>  -             gcc_assert (defstmt);
>  -         }
>           /* We lookup with the LHS, so do not use vn_lookup_or_add_with_stmt
>              here, as that will result in useless reference lookups.  */
>           valvh = vn_lookup_or_add (val);
>  --- 3243,3248 ----
>  *************** compute_avail (void)
>  *** 3562,3568 ****
>             {
>               if (make_values_for_stmt (stmt, block))
>                 continue;
>  -
>             }
>
>           /* For any other statement that we don't recognize, simply
>  --- 3549,3554 ----
>  *************** compute_avail (void)
>  *** 3570,3582 ****
>              AVAIL_OUT and TMP_GEN.  */
>           FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
>             add_to_sets (op, op, NULL, TMP_GEN (block), AVAIL_OUT (block));
>  -
>  -         FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
>  -           {
>  -             add_to_sets (op, op, NULL, NULL , AVAIL_OUT (block));
>  -             if (TREE_CODE (op) == SSA_NAME || can_PRE_operation (op))
>  -               add_to_exp_gen (block, op);
>  -           }
>         }
>
>         /* Put the dominator children of BLOCK on the worklist of blocks
>  --- 3556,3561 ----
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]