Re: [PATCH, 4.5] Make fwprop find out that non-virtual constant member pointers are non-virtual

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Mar 11, 2009 at 3:39 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> the patch below, aimed at 4.5, ?is inspired by PR 3713 (yes, only four
> digits). ?The problem is that when we compile the following simple C++
> code we end ?up with a run-time conditional jump ?(I confirmed this by
> examining the final assembly):
>
> ? ?struct A
> ? ?{
> ? ? ? ? ? ?void foo()
> ? ? ? ? ? ?{
> ? ? ? ? ? ?}
> ? ?};
>
> ? ?int main()
> ? ?{
> ? ? void (A::* const p)() = & A::foo;
> ? ? ? ? ? ?A a;
> ? ? ? ? ? ?(a.*p)();
> ? ?}
>
> The optimized dump looks like:
>
>
> ======================================================================
> void A::foo() (struct A * const this)
> {
> <bb 2>:
> ?return;
>
> }
>
>
>
> ;; Function int main() (main)
>
> int main() ()
> {
> ?A:: * p$__pfn;
> ?struct A a;
> ?int D.1728;
>
> <bb 2>:
> ?D.1728 = (int) foo;
> ?if (D.1728 & 1 != 0)
> ? ?goto <bb 3>;
> ?else
> ? ?goto <bb 5>;
>
> <bb 5>:
> ?p$__pfn = foo;
> ?goto <bb 4>;
>
> <bb 3>:
> ?p$__pfn = (A:: *) *(*(int (*__vtbl_ptr_type) (void) * *) &a + (unsigned int) (D.1728 + -1));
>
> <bb 4>:
> ?p$__pfn (&a);
> ?return 0;
>
> }
> ======================================================================
>
> I believe ?the condition can ?be evaluated at ?run time by ?looking at
> DECL_ALIGN_UNIT ?of foo. ? The patch ?does ?just that ?in the ?forward
> propagation pass and gets rid of the run time conditional jump.

We had this in fold once and that caused PR35705.  You could try to
teach phiprop to hoist the function call to both if arms so one of them
can become a direct call.

Richard.

> On the other ?hand, this patch does not achieve ?what the bug reporter
> wanted in the ?first place since the method foo ?does not get inlined.
> This is because ?this patch requires the member ?pointer to be already
> broken down by SRA ?and we run fwprop before SRA, not ?after it in the
> early ?stage. ?Moreover, ?fwprop after ?SRA did ?not help ?on ?its own
> either, I had to add ccp there too. ?That seems like too much overhead
> for such a test case, we either ?need a better place where to put this
> or do ?some pattern matching along ?the lines of what ?we currently do
> indirect inlining to inline this stuff too.
>
> However, I ?believe that situations like ?these can be ?created by the
> inliner ?(which might ?then decide ?not to ?inline the ?member pointer
> call) and so should be handled ?after inlining too. ?And this seems to
> work.
>
> I have bootstrapped and tested the patch on x86_64. ?Is it OK for
> trunk once stage1 opens again.
>
> Thanks
>
> Martin
>
>
>
> 2009-03-11 ?Martin Jambor ?<mjambor@suse.cz>
>
> ? ? ? ?* tree-ssa-forwprop.c (simplify_bitwise_and): New function.
> ? ? ? ?(tree_ssa_forward_propagate_single_use_vars): Handle assing statements
> ? ? ? ?with BIT_AND_EXPR on the RHS by calling simplify_bitwise_and.
>
> Index: iinln/gcc/tree-ssa-forwprop.c
> ===================================================================
> --- iinln.orig/gcc/tree-ssa-forwprop.c
> +++ iinln/gcc/tree-ssa-forwprop.c
> @@ -147,6 +147,14 @@ along with GCC; see the file COPYING3.
>
> ? ? ?ptr2 = &x[index];
>
> + ?Or
> + ? ?ssa = (int) decl
> + ? ?res = ssa & 1
> +
> + ?Provided that decl has known alignment >= 2, will get turned into
> +
> + ? ?res = 0
> +
> ? We also propagate casts into SWITCH_EXPR and COND_EXPR conditions to
> ? allow us to remove the cast and {NOT_EXPR,NEG_EXPR} into a subsequent
> ? {NOT_EXPR,NEG_EXPR}.
> @@ -1124,6 +1132,50 @@ simplify_gimple_switch (gimple stmt)
> ? ? }
> ?}
>
> +/* Check assignments with a "ssa & 1" on the RHS for operating on addresses
> + ? known to be aligned to at least two. ?If so, the result is known to be
> + ? zero.
> +*/
> +
> +static void
> +simplify_bitwise_and (gimple_stmt_iterator *gsi, gimple stmt)
> +{
> + ?gimple def_stmt;
> + ?tree op = gimple_assign_rhs1 (stmt);
> + ?tree val = gimple_assign_rhs2 (stmt);
> + ?tree def_rhs, deref;
> +
> + ?if (!integer_onep (val)
> + ? ? ?|| TREE_CODE (op) != SSA_NAME)
> + ? ?return;
> +
> + ?def_stmt = SSA_NAME_DEF_STMT (op);
> + ?if (!gimple_assign_cast_p (def_stmt)
> + ? ? ?|| !INTEGRAL_TYPE_P (gimple_expr_type (def_stmt)))
> + ? ?return;
> +
> + ?def_rhs = gimple_assign_rhs1 (def_stmt);
> + ?if (TREE_CODE (def_rhs) != ADDR_EXPR)
> + ? ?return;
> +
> + ?deref = TREE_OPERAND (def_rhs, 0);
> + ?if (DECL_ALIGN_UNIT (deref) < 2)
> + ? ?return;
> +
> + ?if (dump_file)
> + ? ?{
> + ? ? ?fprintf (dump_file,
> + ? ? ? ? ? ? ?"The RHS is always zero, replacing it with a constant: ");
> + ? ? ?print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> + ? ?}
> +
> + ?gimple_assign_set_rhs_from_tree (gsi,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?build_int_cst (gimple_expr_type (stmt), 0));
> +
> + ?update_stmt (stmt);
> + ?return;
> +}
> +
> ?/* Main entry point for the forward propagation optimizer. ?*/
>
> ?static unsigned int
> @@ -1206,6 +1258,11 @@ tree_ssa_forward_propagate_single_use_va
> ? ? ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ? ? ?gsi_next (&gsi);
> ? ? ? ? ? ? ? ?}
> + ? ? ? ? ? ? else if (gimple_assign_rhs_code (stmt) == BIT_AND_EXPR)
> + ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? simplify_bitwise_and (&gsi, stmt);
> + ? ? ? ? ? ? ? ? gsi_next (&gsi);
> + ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ?gsi_next (&gsi);
> ? ? ? ? ? ?}
> Index: iinln/gcc/testsuite/g++.dg/tree-ssa/fwprop-align.C
> ===================================================================
> --- /dev/null
> +++ iinln/gcc/testsuite/g++.dg/tree-ssa/fwprop-align.C
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-forwprop2" } */
> +
> +struct A
> +{
> + ?void foo ()
> + ?{
> + ?}
> +};
> +
> +int main()
> +{
> + ?void (A::* const p)() = & A::foo;
> + ?A a;
> + ?(a.*p)();
> +}
> +
> +/* We should eliminate the check if p points to a virtual function. */
> +/* { dg-final { scan-tree-dump-times "The RHS is always zero, replacing it with a constant" 1 "forwprop2" } } */
> +/* { dg-final { cleanup-tree-dump "forwprop2" } } */
>