On Fri, May 11, 2012 at 10:53 AM, Bin Cheng <bin.ch...@arm.com> wrote:
> Hi,
> I previously noticed from testcases that gcc now does not sink loads from
> memory
> in tree-ssa-sink.c. After discussing I worked out a patch to support this in
> gcc.
> Please refer to http://gcc.gnu.org/ml/gcc/2012-04/msg00404.html for some
> info.
>
> I think it is a trivial optimization, but it might be wanted in GCC since
> less
> instructions are executed with this.
>
> One potential issue might be: Should I disable it when optimizing for size
> and
> the sink hurting it.
>
> I bootstrapped x86 and tested the patch on x86/arm, no new fail introduced.
>
> It is OK for trunk?
> And comments are welcome. Thanks.
First of all, can you do your patches with -p? That makes them easier
to review.
I think you should increase testing coverage - interesting cases include
+extern int b;
+void bar();
+int
+foo (int a, int c)
+{
+ int x = b;
+ if (c == 1)
+ return x;
+ else
{
bar ();
+ return a;
}
+}
where bar () should not prevent sinking.
I don't like the way you perform the checking whether there is a store along
the paths from the source to the destination location - we have virtual operands
in SSA form that should enable a more efficient way to do this. (in general
SSU form would be the best here, as the whole sinking is really partial dead
code elimination) Basically you are looking for the active VUSE operand
at the insert location and want to make sure it matches that of the load you
want to sink. Or the other way around, you are looking for all VDEF operands
that make the loads VUSE operand dead and want to make sure those are
not on any path from the sink source to its destination. Walking upwards
is matching what SSA provides the best (but doesn't help very much, as as
soon that you've found the SSA name to start walking you are finished).
But it boils down to recording the active VUSE at basic-block entry (something
that's the same for all loads, no need to compute that per load).
But, as we are dealing with simple cases (any store will block sinking of loads)
we can as well limit the CFG structure we can sink over to only consider
immediate successors of the sink source. As sink_code_in_bb already
walks backwards over the stmts of the sink source it's easy to remember
whether we've seen a store here or not (also, if there is a relevant store
in the source BB then the vuses SSA uses are all in BB or its dominators
and the single store reachable that way is in BB).
Thanks,
Richard.
>
> gcc/ChangeLog:
> 2012-05-11 Bin Cheng <bin.ch...@arm.com>
>
> * tree-ssa-sink.c (pred_blocks, pred_visited): New static vars.
> (init_sink_load): New function initializes pred_blocks and
> pred_visited.
> (free_sink_load): New function frees pred_blocks and pred_visited.
> (sink_load_p): New function checks whether load operation should be
> sunk.
> (execute_sink_code): Call init_sink_load and free_sink_load.
> (statement_sink_location): Sink loads from memory if possible.
>
> gcc/testsuite/ChangeLog:
> 2012-05-11 Bin Cheng <bin.ch...@arm.com>
>
> * testsuite/gcc.dg/tree-ssa/ssa-sink-10.c: New test.
> * testsuite/gcc.dg/tree-ssa/ssa-sink-11.c: New test.
