Re: [PATCH] local-factoring on TREE-SSA

[Steven Bosscher <stevenb at suse dot de>]

On Tuesday 27 July 2004 20:09, Judit Jasz wrote:
> Hello,
>
> I've modified the patch on the grounds of the review. It produces the same
> result as the former patch.
>
> I would like to ask whether this patch is OK for the mainline now, or
> there are some other issues to be corrected?
>
> Regards,
> Judit Jasz
>
>
> 2004-07-28 Judit Jasz <jasy@inf.u-szeged.hu>
>
>  * tree-ssa-lfact.c: New file.
>  * Makefile.in: (tree-ssa-lfact.o) Add.
>  * common.opt: (ftree-lfact) New flag.
>  * opts.c: (decode_options): Set.
>  * tree-optimize.c: (init_tree_optimization_passes): Add pass_tree_lfact.
>  * tree-pass.h: (pass_tree_lfact): Declare.

Hi Judit,

First of all, I really dislike the fact that this algorithm has O(n^2)
behavior, is that really necessary?

Then, there are a lot of things in your patch that simply don't make
any sense.  Like the following:

+      simple_cst_equal (SSA_NAME_VAR (TREE_OPERAND (var1, 0)),
+                        SSA_NAME_VAR (TREE_OPERAND (var2, 0)));

and

+              simple_cst_equal (TREE_OPERAND (stmt1, 1),
+                                TREE_OPERAND (stmt2, 1)) == 2 ? 1 : 0);

SSA_NAME_VAR is the underlying *_DECL node for the SSA name you seem
to be trying to compare here.  You cannot compare simple_cst_equal for
that, _cst_ stands for "constant" here: it compares constants, not
declarations.  Perhaps you mean operand_equal_p?

But even that would not make sense, you shouldn't be looking at the
declarations in the first place, I doubt that is safe.

I'm not even sure what you're trying to do in the second snippet.

Could you also demonstrate what this patch actually does?  To be honest
with you, I doubt that the patch achieves anything that makes sense at
all in the form that you posted it

(Personally I think this whole transformation should not be done at the
tree level, but instead on RTL.  What happened to that idea??)

I cannot speak for the people with the powers to approve/disprove your
patch, but I am fairly sure you'll find that this patch will not get
accepted until you fix the problems I mentioned above, and until you
show the effect of your patch on the generated code.

Gr.
Steven