| Message ID | 201103211219.30008.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
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 >
> > 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 &&.
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 >
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));
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.