Fix thinko in gimple_assign_set_rhs_with_ops

Fix thinko in gimple_assign_set_rhs_with_ops

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 2929 bytes --]

The attached fix to the Ada front-end introduces a regression in the ACATS 
testsuite for cb4009a.  The backtrace is:

#0  operation_could_trap_helper_p(tree_code, bool, bool, bool, bool, 
tree_node*, bool*) () at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2439
#1  0x00000000012946d7 in stmt_could_throw_1_p (stmt=<optimized out>)
    at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2759
#2  stmt_could_throw_p(gimple*) [clone .part.186] ()
    at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2809
#3  0x0000000001295f28 in stmt_could_throw_p (stmt=0x7ffff6c5d420)
    at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2924
#4  maybe_clean_or_replace_eh_stmt (old_stmt=old_stmt@entry=0x7ffff6c565d8, 
    new_stmt=new_stmt@entry=0x7ffff6c5d420)
    at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2908
#5  0x0000000000fd8a0b in gsi_replace(gimple_stmt_iterator*, gimple*, bool) ()
    at /home/eric/gnat/gnat-head/src/gcc/gimple-iterator.c:447
#6  0x0000000000fd14af in 
gimple_assign_set_rhs_with_ops(gimple_stmt_iterator*, tree_code, tree_node*, 
tree_node*, tree_node*) ()
    at /home/eric/gnat/gnat-head/src/gcc/gimple.c:1616
#7  0x0000000000fe7af7 in replace_stmt_with_simplification (inplace=false, 
    seq=0x7fffffffd818, ops=0x7fffffffd820, rcode=..., gsi=0x7fffffffd8e0)
    at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4151
#8  fold_stmt_1(gimple_stmt_iterator*, bool, tree_node* (*)(tree_node*)) ()
    at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4462
#9  0x0000000000fe961a in fold_stmt (gsi=gsi@entry=0x7fffffffd8e0, 
    valueize=valueize@entry=0x1331170 <fwprop_ssa_val(tree)>)
    at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4689

The folding is turning a TRUNC_DIV_EXPR into a COND_EXPR and the code does:

  /* If the new CODE needs more operands, allocate a new statement.  */
  if (gimple_num_ops (stmt) < new_rhs_ops + 1)
    {
      tree lhs = gimple_assign_lhs (stmt);
      gimple *new_stmt = gimple_alloc (gimple_code (stmt), new_rhs_ops + 1);
      memcpy (new_stmt, stmt, gimple_size (gimple_code (stmt)));
      gimple_init_singleton (new_stmt);
      gsi_replace (gsi, new_stmt, true);
      stmt = new_stmt;

      /* The LHS needs to be reset as this also changes the SSA name
	 on the LHS.  */
      gimple_assign_set_lhs (stmt, lhs);
    }

i.e it asks gsi_replace to update EH info, which doesn't work since the new 
statement is dummy at this point.  Fixed by passing false instead of true.

Bootstrapped/regtested on x86_64-suse-linux, applied on mainline as obvious.


2017-07-25  Eric Botcazou  <ebotcazou@adacore.com>

	* gimple.c (gimple_assign_set_rhs_with_ops): Do not ask gsi_replace
	to update EH info here.


2017-07-25  Javier Miranda  <miranda@adacore.com>

ada/
	* checks.adb (Apply_Divide_Checks): Ensure that operands are not
	evaluated twice. 


2017-07-25  Eric Botcazou  <ebotcazou@adacore.com>

testsuite/
	* gnat.dg/opt66.adb: New test.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1334 bytes --]

Index: gimple.c
===================================================================
--- gimple.c	(revision 250379)
+++ gimple.c	(working copy)
@@ -1613,7 +1613,7 @@ gimple_assign_set_rhs_with_ops (gimple_stmt_iterat
       gimple *new_stmt = gimple_alloc (gimple_code (stmt), new_rhs_ops + 1);
       memcpy (new_stmt, stmt, gimple_size (gimple_code (stmt)));
       gimple_init_singleton (new_stmt);
-      gsi_replace (gsi, new_stmt, true);
+      gsi_replace (gsi, new_stmt, false);
       stmt = new_stmt;
 
       /* The LHS needs to be reset as this also changes the SSA name
Index: ada/checks.adb
===================================================================
--- ada/checks.adb	(revision 250379)
+++ ada/checks.adb	(working copy)
@@ -1818,6 +1818,13 @@ package body Checks is
                      and then
                   ((not LOK) or else (Llo = LLB))
                then
+                  --  Ensure that expressions are not evaluated twice (once
+                  --  for their runtime checks and once for their regular
+                  --  computation).
+
+                  Force_Evaluation (Left, Mode => Strict);
+                  Force_Evaluation (Right, Mode => Strict);
+
                   Insert_Action (N,
                     Make_Raise_Constraint_Error (Loc,
                       Condition =>

[-- Attachment #3: opt66.adb --]
[-- Type: text/x-adasrc, Size: 205 bytes --]

-- { dg-do compile }
-- { dg-options "-O" }

procedure Opt66 (I : Integer) is
  E : exception;
begin
  if I = 0 then
    raise E;
  end if;
  Opt66 (I - I / abs (I));
exception
  when others => null;
end;

Re: Fix thinko in gimple_assign_set_rhs_with_ops

[Richard Biener]

On Tue, Jul 25, 2017 at 3:59 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> The attached fix to the Ada front-end introduces a regression in the ACATS
> testsuite for cb4009a.  The backtrace is:
>
> #0  operation_could_trap_helper_p(tree_code, bool, bool, bool, bool,
> tree_node*, bool*) () at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2439
> #1  0x00000000012946d7 in stmt_could_throw_1_p (stmt=<optimized out>)
>     at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2759
> #2  stmt_could_throw_p(gimple*) [clone .part.186] ()
>     at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2809
> #3  0x0000000001295f28 in stmt_could_throw_p (stmt=0x7ffff6c5d420)
>     at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2924
> #4  maybe_clean_or_replace_eh_stmt (old_stmt=old_stmt@entry=0x7ffff6c565d8,
>     new_stmt=new_stmt@entry=0x7ffff6c5d420)
>     at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2908
> #5  0x0000000000fd8a0b in gsi_replace(gimple_stmt_iterator*, gimple*, bool) ()
>     at /home/eric/gnat/gnat-head/src/gcc/gimple-iterator.c:447
> #6  0x0000000000fd14af in
> gimple_assign_set_rhs_with_ops(gimple_stmt_iterator*, tree_code, tree_node*,
> tree_node*, tree_node*) ()
>     at /home/eric/gnat/gnat-head/src/gcc/gimple.c:1616
> #7  0x0000000000fe7af7 in replace_stmt_with_simplification (inplace=false,
>     seq=0x7fffffffd818, ops=0x7fffffffd820, rcode=..., gsi=0x7fffffffd8e0)
>     at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4151
> #8  fold_stmt_1(gimple_stmt_iterator*, bool, tree_node* (*)(tree_node*)) ()
>     at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4462
> #9  0x0000000000fe961a in fold_stmt (gsi=gsi@entry=0x7fffffffd8e0,
>     valueize=valueize@entry=0x1331170 <fwprop_ssa_val(tree)>)
>     at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4689
>
> The folding is turning a TRUNC_DIV_EXPR into a COND_EXPR and the code does:
>
>   /* If the new CODE needs more operands, allocate a new statement.  */
>   if (gimple_num_ops (stmt) < new_rhs_ops + 1)
>     {
>       tree lhs = gimple_assign_lhs (stmt);
>       gimple *new_stmt = gimple_alloc (gimple_code (stmt), new_rhs_ops + 1);
>       memcpy (new_stmt, stmt, gimple_size (gimple_code (stmt)));
>       gimple_init_singleton (new_stmt);
>       gsi_replace (gsi, new_stmt, true);
>       stmt = new_stmt;
>
>       /* The LHS needs to be reset as this also changes the SSA name
>          on the LHS.  */
>       gimple_assign_set_lhs (stmt, lhs);
>     }
>
> i.e it asks gsi_replace to update EH info, which doesn't work since the new
> statement is dummy at this point.  Fixed by passing false instead of true.
>
> Bootstrapped/regtested on x86_64-suse-linux, applied on mainline as obvious.

I think we should get rid of that update-EH-info flag, always assuming
false.  This
is IMHO a too low-level interface to do this (and we lack a
gsi_replace_without_update)

Not sure if you're willing to do this kind of cleanup ;)

Richard.

>
> 2017-07-25  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gimple.c (gimple_assign_set_rhs_with_ops): Do not ask gsi_replace
>         to update EH info here.
>
>
> 2017-07-25  Javier Miranda  <miranda@adacore.com>
>
> ada/
>         * checks.adb (Apply_Divide_Checks): Ensure that operands are not
>         evaluated twice.
>
>
> 2017-07-25  Eric Botcazou  <ebotcazou@adacore.com>
>
> testsuite/
>         * gnat.dg/opt66.adb: New test.
>
>
> --
> Eric Botcazou

Re: Fix thinko in gimple_assign_set_rhs_with_ops

[Eric Botcazou]

> I think we should get rid of that update-EH-info flag, always assuming
> false.  This is IMHO a too low-level interface to do this (and we lack a
> gsi_replace_without_update)

This looks doable (there is only one call to gsi_replace that tests the return 
value) but the current interface is probably the most convenient.

-- 
Eric Botcazou