[patch] Couple of tweaks to the gimplifier

[patch] Couple of tweaks to the gimplifier

[Eric Botcazou]

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

Hi,

the attached patch makes a couple of tweaks to the gimplifier in order to help 
Ada, but I think that they are of general usefulness:

  1) Set TREE_THIS_NOTRAP on the INDIRECT_REF built for VLA decls.  This is
     correct since stack memory isn't considered as trapping in the IL.

  2) Improve gimplification of complex conditions in COND_EXPR.  They are
     naturally generated by the Ada compiler and the patch avoids emitting
     redundant branches in GIMPLE, visible at -O0 for the testcase:

procedure P (B : Boolean; S1, S2 : String) is
begin
  if B and then S1 & S2 = "toto" then
    raise Program_Error;
  end if;
end;

@@ -158,21 +158,12 @@
 	movl	%r12d, %eax
 	subl	%ebx, %eax
 	cmpl	$3, %eax
-	jne	.L33
+	jne	.L18
 	.loc 1 3 0 discriminator 1
 	movq	-40(%rbp), %rax
 	movl	(%rax), %eax
 	cmpl	$1869901684, %eax
-	jne	.L33
-	.loc 1 3 0 discriminator 2
-	movl	$1, %eax
-	jmp	.L34
-.L33:
-	movl	$0, %eax
-.L34:
-	.loc 1 3 0 discriminator 3
-	testb	%al, %al
-	je	.L18
+	jne	.L18
 	.loc 1 4 0 is_stmt 1
 	movl	$4, %esi
 	movl	$.LC0, %edi

Bootstrapped/regtested on x86_64-suse-linux, OK for the mainline?


2011-03-21  Eric Botcazou  <ebotcazou@adacore.com>

	* gimplify.c (gimplify_vla_decl): Set TREE_THIS_NOTRAP flag.
	(gimplify_cond_expr): Gimplify COMPOUND_EXPR conditions.


-- 
Eric Botcazou

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

Index: gimplify.c
===================================================================
--- gimplify.c	(revision 171044)
+++ gimplify.c	(working copy)
@@ -1322,6 +1322,7 @@ gimplify_vla_decl (tree decl, gimple_seq
   addr = create_tmp_var (ptr_type, get_name (decl));
   DECL_IGNORED_P (addr) = 0;
   t = build_fold_indirect_ref (addr);
+  TREE_THIS_NOTRAP (t) = 1;
   SET_DECL_VALUE_EXPR (decl, t);
   DECL_HAS_VALUE_EXPR_P (decl) = 1;
 
@@ -2981,6 +2982,11 @@ gimplify_cond_expr (tree *expr_p, gimple
       return GS_ALL_DONE;
     }
 
+  /* Remove any COMPOUND_EXPR so the following cases will be caught.  */
+  STRIP_TYPE_NOPS (TREE_OPERAND (expr, 0));
+  if (TREE_CODE (TREE_OPERAND (expr, 0)) == COMPOUND_EXPR)
+    gimplify_compound_expr (&TREE_OPERAND (expr, 0), pre_p, true);
+
   /* Make sure the condition has BOOLEAN_TYPE.  */
   TREE_OPERAND (expr, 0) = gimple_boolify (TREE_OPERAND (expr, 0));
 

Re: [patch] Couple of tweaks to the gimplifier

[Richard Guenther]

On Mon, Mar 21, 2011 at 12:19 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> the attached patch makes a couple of tweaks to the gimplifier in order to help
> Ada, but I think that they are of general usefulness:
>
>  1) Set TREE_THIS_NOTRAP on the INDIRECT_REF built for VLA decls.  This is
>     correct since stack memory isn't considered as trapping in the IL.

This is ok.

>  2) Improve gimplification of complex conditions in COND_EXPR.  They are
>     naturally generated by the Ada compiler and the patch avoids emitting
>     redundant branches in GIMPLE, visible at -O0 for the testcase:

Shouldn't

+  /* Remove any COMPOUND_EXPR so the following cases will be caught.  */
+  STRIP_TYPE_NOPS (TREE_OPERAND (expr, 0));
+  if (TREE_CODE (TREE_OPERAND (expr, 0)) == COMPOUND_EXPR)
+    gimplify_compound_expr (&TREE_OPERAND (expr, 0), pre_p, true);

happen in gimple_boolify instead so that other callers also benefit?
That is, add a COMPOUND_EXPR case there?

> procedure P (B : Boolean; S1, S2 : String) is
> begin
>  if B and then S1 & S2 = "toto" then
>    raise Program_Error;
>  end if;
> end;

So, what does the GENERIC look like here?

Thanks,
Richard.


> @@ -158,21 +158,12 @@
>        movl    %r12d, %eax
>        subl    %ebx, %eax
>        cmpl    $3, %eax
> -       jne     .L33
> +       jne     .L18
>        .loc 1 3 0 discriminator 1
>        movq    -40(%rbp), %rax
>        movl    (%rax), %eax
>        cmpl    $1869901684, %eax
> -       jne     .L33
> -       .loc 1 3 0 discriminator 2
> -       movl    $1, %eax
> -       jmp     .L34
> -.L33:
> -       movl    $0, %eax
> -.L34:
> -       .loc 1 3 0 discriminator 3
> -       testb   %al, %al
> -       je      .L18
> +       jne     .L18
>        .loc 1 4 0 is_stmt 1
>        movl    $4, %esi
>        movl    $.LC0, %edi
>
> Bootstrapped/regtested on x86_64-suse-linux, OK for the mainline?
>
>
> 2011-03-21  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * gimplify.c (gimplify_vla_decl): Set TREE_THIS_NOTRAP flag.
>        (gimplify_cond_expr): Gimplify COMPOUND_EXPR conditions.
>
>
> --
> Eric Botcazou
>

Re: [patch] Couple of tweaks to the gimplifier

[Eric Botcazou]

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

> >  1) Set TREE_THIS_NOTRAP on the INDIRECT_REF built for VLA decls.  This
> > is correct since stack memory isn't considered as trapping in the IL.
>
> This is ok.

Thanks.

> >  2) Improve gimplification of complex conditions in COND_EXPR.  They are
> >     naturally generated by the Ada compiler and the patch avoids emitting
> >     redundant branches in GIMPLE, visible at -O0 for the testcase:
>
> Shouldn't
>
> +  /* Remove any COMPOUND_EXPR so the following cases will be caught.  */
> +  STRIP_TYPE_NOPS (TREE_OPERAND (expr, 0));
> +  if (TREE_CODE (TREE_OPERAND (expr, 0)) == COMPOUND_EXPR)
> +    gimplify_compound_expr (&TREE_OPERAND (expr, 0), pre_p, true);
>
> happen in gimple_boolify instead so that other callers also benefit?
> That is, add a COMPOUND_EXPR case there?

Not clear to me.  gimple_boolify doesn't gimplify, it boolifies, i.e. only does 
type conversions to boolean.  This looks orthogonal.

> So, what does the GENERIC look like here?

Attached.  Barely readable, like pretty much all GENERIC for Ada, but you can 
see the big IF statement with the COMPOUND_EXPR on the RHS of the &&.

-- 
Eric Botcazou

[-- Attachment #2: p.adb.003t.original --]
[-- Type: text/x-csrc, Size: 5038 bytes --]

P (const boolean b, struct  s1, struct  s2)
{
  SAVE_EXPR <s2.P_BOUNDS->LB0>;
  SAVE_EXPR <s2.P_BOUNDS->UB0>;
  SAVE_EXPR <SAVE_EXPR <s2.P_BOUNDS->UB0> >= SAVE_EXPR <s2.P_BOUNDS->LB0> ? (SAVE_EXPR <s2.P_BOUNDS->UB0> - SAVE_EXPR <s2.P_BOUNDS->LB0>) + 1 : 0>;
  SAVE_EXPR <s1.P_BOUNDS->LB0>;
  SAVE_EXPR <s1.P_BOUNDS->UB0>;
  SAVE_EXPR <SAVE_EXPR <s1.P_BOUNDS->UB0> >= SAVE_EXPR <s1.P_BOUNDS->LB0> ? (SAVE_EXPR <s1.P_BOUNDS->UB0> - SAVE_EXPR <s1.P_BOUNDS->LB0>) + 1 : 0>;
  {
    typedef p__TS2bP1___XD p__TS2bP1___XD;
    typedef <unnamed-signed:64> struct <unnamed-signed:64>;
    typedef character p__S2b[(size_type) SAVE_EXPR <s2.P_BOUNDS->LB0>:SAVE_EXPR <s2.P_BOUNDS->UB0> >= SAVE_EXPR <s2.P_BOUNDS->LB0> ? (size_type) SAVE_EXPR <s2.P_BOUNDS->UB0> : (size_type) SAVE_EXPR <s2.P_BOUNDS->LB0> + -1];
    typedef p__TS1bP1___XD p__TS1bP1___XD;
    typedef <unnamed-signed:64> struct <unnamed-signed:64>;
    typedef character p__S1b[(size_type) SAVE_EXPR <s1.P_BOUNDS->LB0>:SAVE_EXPR <s1.P_BOUNDS->UB0> >= SAVE_EXPR <s1.P_BOUNDS->LB0> ? (size_type) SAVE_EXPR <s1.P_BOUNDS->UB0> : (size_type) SAVE_EXPR <s1.P_BOUNDS->LB0> + -1];
    const integer L3b = (const integer) (SAVE_EXPR <SAVE_EXPR <s1.P_BOUNDS->UB0> >= SAVE_EXPR <s1.P_BOUNDS->LB0> ? (SAVE_EXPR <s1.P_BOUNDS->UB0> - SAVE_EXPR <s1.P_BOUNDS->LB0>) + 1 : 0>);
    const integer L4b = (const integer) (SAVE_EXPR <SAVE_EXPR <s2.P_BOUNDS->UB0> >= SAVE_EXPR <s2.P_BOUNDS->LB0> ? (SAVE_EXPR <s2.P_BOUNDS->UB0> - SAVE_EXPR <s2.P_BOUNDS->LB0>) + 1 : 0>);
    const integer L5b = (const integer) ((integer) L3b + (integer) L4b);
    const integer L6b = (integer) L3b != 0 ? (const integer) SAVE_EXPR <s1.P_BOUNDS->LB0> : (const integer) SAVE_EXPR <s2.P_BOUNDS->LB0>;
    const integer R8b = (integer) L5b == 0 ? (const integer) SAVE_EXPR <s2.P_BOUNDS->UB0> : (const integer) (((integer) L5b + -1) + (integer) L6b);
    typedef p__TTS7bSP1___XD p__TTS7bSP1___XD;
    typedef <unnamed-signed:64> struct <unnamed-signed:64>;
    typedef character p__TS7bS[(size_type) L6b:(integer) R8b >= (integer) L6b ? (size_type) R8b : (size_type) L6b + -1];
    character S7b[(size_type) L6b:(integer) R8b >= (integer) L6b ? (size_type) R8b : (size_type) L6b + -1];
    typedef <unnamed-signed:64> struct <unnamed-signed:64>;
    typedef character p__T9b[1:4];

        typedef p__TS2bP1___XD p__TS2bP1___XD;
        typedef <unnamed-signed:64> struct <unnamed-signed:64>;
        typedef character p__S2b[(size_type) SAVE_EXPR <s2.P_BOUNDS->LB0>:SAVE_EXPR <s2.P_BOUNDS->UB0> >= SAVE_EXPR <s2.P_BOUNDS->LB0> ? (size_type) SAVE_EXPR <s2.P_BOUNDS->UB0> : (size_type) SAVE_EXPR <s2.P_BOUNDS->LB0> + -1];
        typedef p__TS1bP1___XD p__TS1bP1___XD;
        typedef <unnamed-signed:64> struct <unnamed-signed:64>;
        typedef character p__S1b[(size_type) SAVE_EXPR <s1.P_BOUNDS->LB0>:SAVE_EXPR <s1.P_BOUNDS->UB0> >= SAVE_EXPR <s1.P_BOUNDS->LB0> ? (size_type) SAVE_EXPR <s1.P_BOUNDS->UB0> : (size_type) SAVE_EXPR <s1.P_BOUNDS->LB0> + -1];
        typedef <unnamed-signed:64> struct <unnamed-signed:64>;
        typedef character p__T9b[1:4];
    if ((boolean) b && (SAVE_EXPR <    const integer L3b = (const integer) (SAVE_EXPR <SAVE_EXPR <s1.P_BOUNDS->UB0> >= SAVE_EXPR <s1.P_BOUNDS->LB0> ? (SAVE_EXPR <s1.P_BOUNDS->UB0> - SAVE_EXPR <s1.P_BOUNDS->LB0>) + 1 : 0>);
        const integer L4b = (const integer) (SAVE_EXPR <SAVE_EXPR <s2.P_BOUNDS->UB0> >= SAVE_EXPR <s2.P_BOUNDS->LB0> ? (SAVE_EXPR <s2.P_BOUNDS->UB0> - SAVE_EXPR <s2.P_BOUNDS->LB0>) + 1 : 0>);
        const integer L5b = (const integer) ((integer) L3b + (integer) L4b);
        const integer L6b = (integer) L3b != 0 ? (const integer) SAVE_EXPR <s1.P_BOUNDS->LB0> : (const integer) SAVE_EXPR <s2.P_BOUNDS->LB0>;
        const integer R8b = (integer) L5b == 0 ? (const integer) SAVE_EXPR <s2.P_BOUNDS->UB0> : (const integer) (((integer) L5b + -1) + (integer) L6b);
        typedef p__TTS7bSP1___XD p__TTS7bSP1___XD;
        typedef <unnamed-signed:64> struct <unnamed-signed:64>;
        typedef character p__TS7bS[(size_type) L6b:(integer) R8b >= (integer) L6b ? (size_type) R8b : (size_type) L6b + -1];
        character S7b[(size_type) L6b:(integer) R8b >= (integer) L6b ? (size_type) R8b : (size_type) L6b + -1];
    system.concat_2.str_concat_2 ({.P_ARRAY=(character[(size_type) <PLACEHOLDER_EXPR struct >.P_BOUNDS->LB0:<PLACEHOLDER_EXPR struct >.P_BOUNDS->UB0 >= <PLACEHOLDER_EXPR struct >.P_BOUNDS->LB0 ? (size_type) <PLACEHOLDER_EXPR struct >.P_BOUNDS->UB0 : (size_type) <PLACEHOLDER_EXPR struct >.P_BOUNDS->LB0 + -1] *) &S7b, .P_BOUNDS=&{.LB0=(integer) L6b, .UB0=(integer) R8b}}, s1, s2);>;, (integer) R8b - (integer) L6b == 3 && VIEW_CONVERT_EXPR<character[1:4]>(S7b) == "toto";))
      {
        .gnat_rcheck_21 ("p.adb", 4);
      }
    else
      {
        
      }
    return;
  }


_GLOBAL.SZ0.ada_p (integer p0, integer p1)
{
  return p1 <= p0 ? (bit_size_type) ((((size_type) p0 - (size_type) p1) + 1) * 8) : 0;


_GLOBAL.SZ1.ada_p (integer p0, integer p1)
{
  return p1 <= p0 ? ((size_type) p0 - (size_type) p1) + 1 : 0;

Re: [patch] Couple of tweaks to the gimplifier

[Richard Guenther]

On Mon, Mar 21, 2011 at 6:48 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> >  1) Set TREE_THIS_NOTRAP on the INDIRECT_REF built for VLA decls.  This
>> > is correct since stack memory isn't considered as trapping in the IL.
>>
>> This is ok.
>
> Thanks.
>
>> >  2) Improve gimplification of complex conditions in COND_EXPR.  They are
>> >     naturally generated by the Ada compiler and the patch avoids emitting
>> >     redundant branches in GIMPLE, visible at -O0 for the testcase:
>>
>> Shouldn't
>>
>> +  /* Remove any COMPOUND_EXPR so the following cases will be caught.  */
>> +  STRIP_TYPE_NOPS (TREE_OPERAND (expr, 0));
>> +  if (TREE_CODE (TREE_OPERAND (expr, 0)) == COMPOUND_EXPR)
>> +    gimplify_compound_expr (&TREE_OPERAND (expr, 0), pre_p, true);
>>
>> happen in gimple_boolify instead so that other callers also benefit?
>> That is, add a COMPOUND_EXPR case there?
>
> Not clear to me.  gimple_boolify doesn't gimplify, it boolifies, i.e. only does
> type conversions to boolean.  This looks orthogonal.
>
>> So, what does the GENERIC look like here?
>
> Attached.  Barely readable, like pretty much all GENERIC for Ada, but you can
> see the big IF statement with the COMPOUND_EXPR on the RHS of the &&.

So looking at the GENERIC I fail to see how the patch would handle
the COMPOUND_EXPR which is in operand 1 of the &&.  That's also
one reason I suggested gimple_boolify instead, as that works recursively
on the predicate.  Of course you are right, gimple_boolify doesn't seem to
be prepared to do gimplification.

Ok, debugging.

Ah, I see - we recursively gimplify the pieces of the predicate.  So yes,
I think your patch makes sense.

Thus, ok.

Thanks,
Richard.

> --
> Eric Botcazou
>