RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

[Jason Merrill]

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

I've had a TODO in genericize_cp_loop for a long time suggesting that we 
should genericize to LOOP_EXPR rather than gotos, and now that I need to 
interpret the function body for constexpr evaluation, making this change 
will also simplify that handling.

This change also does away with canonicalizing the condition to the 
bottom of the loop, to avoid the extra goto.  It seems to me that this 
is unnecessary nowadays, since the optimizers are very capable of making 
any necessary transformations, but I'm interested in feedback from other 
people.

Tested x86_64-pc-linux-gnu.

Opinions?

[-- Attachment #2: loop_expr.patch --]
[-- Type: text/x-patch, Size: 4386 bytes --]

commit 1a45860e7757ee054f6bf98bee4ebe5c661dfb90
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Nov 13 23:54:48 2014 -0500

    	* cp-gimplify.c (genericize_cp_loop): Use LOOP_EXPR.
    	(genericize_for_stmt): Handle null statement-list.

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index e5436bb..81b26d2 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -208,7 +208,7 @@ genericize_cp_loop (tree *stmt_p, location_t start_locus, tree cond, tree body,
 		    void *data)
 {
   tree blab, clab;
-  tree entry = NULL, exit = NULL, t;
+  tree exit = NULL;
   tree stmt_list = NULL;
 
   blab = begin_bc_block (bc_break, start_locus);
@@ -222,64 +222,46 @@ genericize_cp_loop (tree *stmt_p, location_t start_locus, tree cond, tree body,
   cp_walk_tree (&incr, cp_genericize_r, data, NULL);
   *walk_subtrees = 0;
 
-  /* If condition is zero don't generate a loop construct.  */
-  if (cond && integer_zerop (cond))
+  if (cond && TREE_CODE (cond) != INTEGER_CST)
     {
-      if (cond_is_first)
-	{
-	  t = build1_loc (start_locus, GOTO_EXPR, void_type_node,
-			  get_bc_label (bc_break));
-	  append_to_statement_list (t, &stmt_list);
-	}
-    }
-  else
-    {
-      /* Expand to gotos, just like c_finish_loop.  TODO: Use LOOP_EXPR.  */
-      tree top = build1 (LABEL_EXPR, void_type_node,
-			 create_artificial_label (start_locus));
-
-      /* If we have an exit condition, then we build an IF with gotos either
-	 out of the loop, or to the top of it.  If there's no exit condition,
-	 then we just build a jump back to the top.  */
-      exit = build1 (GOTO_EXPR, void_type_node, LABEL_EXPR_LABEL (top));
-
-      if (cond && !integer_nonzerop (cond))
-	{
-	  /* Canonicalize the loop condition to the end.  This means
-	     generating a branch to the loop condition.  Reuse the
-	     continue label, if possible.  */
-	  if (cond_is_first)
-	    {
-	      if (incr)
-		{
-		  entry = build1 (LABEL_EXPR, void_type_node,
-				  create_artificial_label (start_locus));
-		  t = build1_loc (start_locus, GOTO_EXPR, void_type_node,
-				  LABEL_EXPR_LABEL (entry));
-		}
-	      else
-		t = build1_loc (start_locus, GOTO_EXPR, void_type_node,
-				get_bc_label (bc_continue));
-	      append_to_statement_list (t, &stmt_list);
-	    }
-
-	  t = build1 (GOTO_EXPR, void_type_node, get_bc_label (bc_break));
-	  exit = fold_build3_loc (start_locus,
-				  COND_EXPR, void_type_node, cond, exit, t);
-	}
-
-      append_to_statement_list (top, &stmt_list);
+      /* If COND is constant, don't bother building an exit.  If it's false,
+	 we won't build a loop.  If it's true, any exits are in the body.  */
+      location_t cloc = EXPR_LOC_OR_LOC (cond, start_locus);
+      exit = build1_loc (cloc, GOTO_EXPR, void_type_node,
+			 get_bc_label (bc_break));
+      exit = fold_build3_loc (cloc, COND_EXPR, void_type_node, cond,
+			      build_empty_stmt (cloc), exit);
     }
 
+  if (exit && cond_is_first)
+    append_to_statement_list (exit, &stmt_list);
   append_to_statement_list (body, &stmt_list);
   finish_bc_block (&stmt_list, bc_continue, clab);
   append_to_statement_list (incr, &stmt_list);
-  append_to_statement_list (entry, &stmt_list);
-  append_to_statement_list (exit, &stmt_list);
+  if (exit && !cond_is_first)
+    append_to_statement_list (exit, &stmt_list);
+
+  if (!stmt_list)
+    stmt_list = build_empty_stmt (start_locus);
+
+  tree loop;
+  if (cond && integer_zerop (cond))
+    {
+      if (cond_is_first)
+	loop = fold_build3_loc (start_locus, COND_EXPR,
+				void_type_node, cond, stmt_list,
+				build_empty_stmt (start_locus));
+      else
+	loop = stmt_list;
+    }
+  else
+    loop = build1_loc (start_locus, LOOP_EXPR, void_type_node, stmt_list);
+
+  stmt_list = NULL;
+  append_to_statement_list (loop, &stmt_list);
   finish_bc_block (&stmt_list, bc_break, blab);
-
-  if (stmt_list == NULL_TREE)
-    stmt_list = build1 (NOP_EXPR, void_type_node, integer_zero_node);
+  if (!stmt_list)
+    stmt_list = build_empty_stmt (start_locus);
 
   *stmt_p = stmt_list;
 }
@@ -303,6 +285,8 @@ genericize_for_stmt (tree *stmt_p, int *walk_subtrees, void *data)
   genericize_cp_loop (&loop, EXPR_LOCATION (stmt), FOR_COND (stmt),
 		      FOR_BODY (stmt), FOR_EXPR (stmt), 1, walk_subtrees, data);
   append_to_statement_list (loop, &expr);
+  if (expr == NULL_TREE)
+    expr = loop;
   *stmt_p = expr;
 }
 

Re: RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

[Richard Biener]

On Sun, Nov 16, 2014 at 6:04 AM, Jason Merrill <jason@redhat.com> wrote:
> I've had a TODO in genericize_cp_loop for a long time suggesting that we
> should genericize to LOOP_EXPR rather than gotos, and now that I need to
> interpret the function body for constexpr evaluation, making this change
> will also simplify that handling.
>
> This change also does away with canonicalizing the condition to the bottom
> of the loop, to avoid the extra goto.  It seems to me that this is
> unnecessary nowadays, since the optimizers are very capable of making any
> necessary transformations, but I'm interested in feedback from other people.
>
> Tested x86_64-pc-linux-gnu.
>
> Opinions?

Lowering less is always nice though I didn't even know GENERIC has
LOOP_EXPR...

Fortran seems to use it though.

Did you inspect generated code for a few testcases?

Thanks,
Richard.

Re: RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

[Jason Merrill]

On 11/17/2014 05:26 AM, Richard Biener wrote:
> Did you inspect generated code for a few testcases?

The generated code for g++.dg/torture/pr37922.C is pretty different at 
-O2, but it's hard for me to tell whether the changes are good, bad, or 
neutral.

Jason

Re: RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

[Richard Biener]

On Mon, Nov 17, 2014 at 4:25 PM, Jason Merrill <jason@redhat.com> wrote:
> On 11/17/2014 05:26 AM, Richard Biener wrote:
>>
>> Did you inspect generated code for a few testcases?
>
>
> The generated code for g++.dg/torture/pr37922.C is pretty different at -O2,
> but it's hard for me to tell whether the changes are good, bad, or neutral.

There is always the possibility of running the C++ portion of SPEC CPU 2006...

Richard.

> Jason
>

Re: RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

[Jason Merrill]

On 11/17/2014 10:27 AM, Richard Biener wrote:
>> The generated code for g++.dg/torture/pr37922.C is pretty different at -O2,
>> but it's hard for me to tell whether the changes are good, bad, or neutral.
>
> There is always the possibility of running the C++ portion of SPEC CPU 2006...

I ran the tramp3d benchmark over 500 iterations before and after the 
change and couldn't see any measurable difference in runtime.  The 
binary with my change was 0.4% smaller.

I'm going to go ahead and check it in; if a performance hit shows up on 
the automated testing we can revisit the choice.

RE: RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

[Bernd Edlinger]

Hi Jason,

I managed to reproduce this fault now.

and entered a bug tracker for it:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64265

any ideas how this patch could move the __tsan_func_entry into the loop?


Thanks
Bernd.

On Wed, 10 Dec 2014 00:10:07, Bernd Edlinger wrote:
>
> Hi Jason,
>
>> I ran the tramp3d benchmark over 500 iterations before and after the
> change and couldn't see any measurable difference in runtime. The
> binary with my
>> change was 0.4% smaller.
> I'm going to go ahead and check it in; if a performance hit shows up on
> the automated testing we can revisit the choice.
>
> Unfortunately, this checkin broke the thread sanitizer:
>
> r217669 | jason | 2014-11-17 20:08:02 +0100 (Mo, 17. Nov 2014) | 2 Zeilen
>
>         * cp-gimplify.c (genericize_cp_loop): Use LOOP_EXPR.
>         (genericize_for_stmt): Handle null statement-list.
>
>
> therefore I would kindly ask you to revert this again.
>
> After that patch some C++ functions moved the call to the __tsan_func_entry into the loop.
> And we get crashes or major memory leaks from this, if the software is compiled with -fsanitize=thread.
>
> This happens in a software package on which I currently work. It is called Softing OPC UA Toolbox.
>
> I found this in the generated assembler code by bisection:
>
> 000000000092747d <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_>:
>   92747d:       55                      push   %rbp
>   92747e:       48 89 e5                mov    %rsp,%rbp
>   927481:       53                      push   %rbx
>   927482:       48 83 ec 18             sub    $0x18,%rsp
>   927486:       48 89 7d e8             mov    %rdi,-0x18(%rbp)
>   92748a:       48 89 75 e0             mov    %rsi,-0x20(%rbp)
>   92748e:       48 8b 45 08             mov    0x8(%rbp),%rax
>   927492:       48 89 c7                mov    %rax,%rdi
>   927495:       e8 26 33 fe ff          callq  90a7c0 <__tsan_func_entry@plt>
>   92749a:       48 8b 45 e8             mov    -0x18(%rbp),%rax
>   92749e:       48 3b 45 e0             cmp    -0x20(%rbp),%rax
>   9274a2:       74 3d                   je     9274e1 <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x64>
>   9274a4:       48 8b 5d e8             mov    -0x18(%rbp),%rbx
>   9274a8:       48 89 d8                mov    %rbx,%rax
>   9274ab:       48 85 db                test   %rbx,%rbx
>   9274ae:       74 0b                   je     9274bb <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x3e>
>   9274b0:       48 89 c2                mov    %rax,%rdx
>   9274b3:       83 e2 07                and    $0x7,%edx
>   9274b6:       48 85 d2                test   %rdx,%rdx
>   9274b9:       74 0f                   je     9274ca <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x4d>
>   9274bb:       48 89 c6                mov    %rax,%rsi
>   9274be:       48 8d 3d 1b a3 f8 00    lea    0xf8a31b(%rip),%rdi        # 18b17e0 <DW.ref.__gxx_personality_v0+0x11878>
>   9274c5:       e8 06 36 fe ff          callq  90aad0 <__ubsan_handle_type_mismatch@plt>
>   9274ca:       48 89 df                mov    %rbx,%rdi
>   9274cd:       e8 1b 09 00 00          callq  927ded <_ZSt11__addressofIN18SoftingOPCToolbox55ValueEEPT_RS2_>
>   9274d2:       48 89 c7                mov    %rax,%rdi
>   9274d5:       e8 21 09 00 00          callq  927dfb <_ZSt8_DestroyIN18SoftingOPCToolbox55ValueEEvPT_>
>   9274da:       48 83 45 e8 20          addq   $0x20,-0x18(%rbp)
>   9274df:       eb ad                   jmp    92748e <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x11>
>   9274e1:       e8 da 2c fe ff          callq  90a1c0 <__tsan_func_exit@plt>
>   9274e6:       48 83 c4 18             add    $0x18,%rsp
>   9274ea:       5b                      pop    %rbx
>   9274eb:       5d                      pop    %rbp
>   9274ec:       c3                      retq
>   9274ed:       90                      nop
>
>
> see the jmp at 9274df: it jumps to _before_ the tsan_func_entry.
> I am not sure how to locate the source code of the above assembler section.
> But I'd guess, it must be some kind of automatically generated default destructor.
>
> All I can say in the moment, that it is was working perfectly before Nov 17.
>
>
> Thanks
> Bernd.
>
 		 	   		  

Re: RFC: PATCH to genericize C++ loops to LOOP_EXPR instead of gotos

[Bernd Edlinger]

Hi Jason,

> I ran the tramp3d benchmark over 500 iterations before and after the 
change and couldn't see any measurable difference in runtime.  The 
binary with my
> change was 0.4% smaller.
I'm going to go ahead and check it in; if a performance hit shows up on 
the automated testing we can revisit the choice.

Unfortunately, this checkin broke the thread sanitizer:

r217669 | jason | 2014-11-17 20:08:02 +0100 (Mo, 17. Nov 2014) | 2 Zeilen

        * cp-gimplify.c (genericize_cp_loop): Use LOOP_EXPR.
        (genericize_for_stmt): Handle null statement-list.


therefore I would kindly ask you to revert this again.

After that patch some C++ functions moved the call to the __tsan_func_entry into the loop.
And we get crashes or major memory leaks from this, if the software is compiled with -fsanitize=thread.

This happens in a software package on which I currently work. It is called Softing OPC UA Toolbox.

I found this in the generated assembler code by bisection:

000000000092747d <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_>:
  92747d:       55                      push   %rbp
  92747e:       48 89 e5                mov    %rsp,%rbp
  927481:       53                      push   %rbx
  927482:       48 83 ec 18             sub    $0x18,%rsp
  927486:       48 89 7d e8             mov    %rdi,-0x18(%rbp)
  92748a:       48 89 75 e0             mov    %rsi,-0x20(%rbp)
  92748e:       48 8b 45 08             mov    0x8(%rbp),%rax
  927492:       48 89 c7                mov    %rax,%rdi
  927495:       e8 26 33 fe ff          callq  90a7c0 <__tsan_func_entry@plt>
  92749a:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  92749e:       48 3b 45 e0             cmp    -0x20(%rbp),%rax
  9274a2:       74 3d                   je     9274e1 <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x64>
  9274a4:       48 8b 5d e8             mov    -0x18(%rbp),%rbx
  9274a8:       48 89 d8                mov    %rbx,%rax
  9274ab:       48 85 db                test   %rbx,%rbx
  9274ae:       74 0b                   je     9274bb <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x3e>
  9274b0:       48 89 c2                mov    %rax,%rdx
  9274b3:       83 e2 07                and    $0x7,%edx
  9274b6:       48 85 d2                test   %rdx,%rdx
  9274b9:       74 0f                   je     9274ca <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x4d>
  9274bb:       48 89 c6                mov    %rax,%rsi
  9274be:       48 8d 3d 1b a3 f8 00    lea    0xf8a31b(%rip),%rdi        # 18b17e0 <DW.ref.__gxx_personality_v0+0x11878>
  9274c5:       e8 06 36 fe ff          callq  90aad0 <__ubsan_handle_type_mismatch@plt>
  9274ca:       48 89 df                mov    %rbx,%rdi
  9274cd:       e8 1b 09 00 00          callq  927ded <_ZSt11__addressofIN18SoftingOPCToolbox55ValueEEPT_RS2_>
  9274d2:       48 89 c7                mov    %rax,%rdi
  9274d5:       e8 21 09 00 00          callq  927dfb <_ZSt8_DestroyIN18SoftingOPCToolbox55ValueEEvPT_>
  9274da:       48 83 45 e8 20          addq   $0x20,-0x18(%rbp)
  9274df:       eb ad                   jmp    92748e <_ZNSt12_Destroy_auxILb0EE9__destroyIPN18SoftingOPCToolbox55ValueEEEvT_S5_+0x11>
  9274e1:       e8 da 2c fe ff          callq  90a1c0 <__tsan_func_exit@plt>
  9274e6:       48 83 c4 18             add    $0x18,%rsp
  9274ea:       5b                      pop    %rbx
  9274eb:       5d                      pop    %rbp
  9274ec:       c3                      retq
  9274ed:       90                      nop


see the jmp at 9274df: it jumps to _before_ the tsan_func_entry.
I am not sure how to locate the source code of the above assembler section.
But I'd guess, it must be some kind of automatically generated default destructor. 

All I can say in the moment, that it is was working perfectly before Nov 17.


Thanks
Bernd.