| Message ID | 16609465.DasV8VXWIg@polaris |
|---|---|
| State | New |
| Headers | show |
On 10/14/2015 09:59 AM, Eric Botcazou wrote: > Hi, > > this is the regression of ACATS c37213k at -O2 with an upcoming change in > the front-end of the Ada compiler: > > eric@polaris:~/gnat/gnat-head/native> gcc/gnat1 -quiet c37213k.adb -I > /home/eric/gnat/bugs/support -O2 > +===========================GNAT BUG DETECTED==============================+ > | Pro 7.4.0w (20151014-60) (x86_64-suse-linux) GCC error: | > | tree check: expected class 'expression', have | > | 'exceptional' (ssa_name) in tree_operand_check, at tree.h:3431| > | Error detected around c37213k.adb:95:37 > > It's recompute_tree_invariant_for_addr_expr receiving an SSA_NAME instead of > an ADDR_EXPR when called from gimplify_addr_expr. The sequence is as follows: > we start with this GIMPLE statement: > > *R.43_60 = c37213k.B_1.B_6.proc6.B_4.B_5.value (); [static-chain: &FRAME.60] > [return slot optimization] > > Then IPA clones the function and turns the statement into: > > MEM[(struct c37213k__B_1__B_6__proc6__B_4__nrec *)R.43_46] = > c37213k.B_1.B_6.proc6.B_4.B_5.value (); [static-chain: &FRAME.60] [return slot > optimization] > > The 'value' function has been NRVed and contains: > > .builtin_memcpy (&<retval>, _9, _10); > > and gets inlined so the above statement is rewritten into: > > .builtin_memcpy (&MEM[(struct c37213k__B_1__B_6__proc6__B_4__nrec *)R.43_46], > _174, _175); > > so gimplify_addr_expr is invoked on: > > &MEM[(struct c37213k__B_1__B_6__proc6__B_4__nrec *)R.43_46] > > and gets confused because it doesn't see that it's just R.43_46 (it would have > seen it if the original INDIRECT_REF was still present in lieu of MEM_REF). > > Hence the attached fixlet. Tested on x86_64-suse-linux, OK for the mainline? > > > 2015-10-14 Eric Botcazou <ebotcazou@adacore.com> > > * gimplify.c (gimplify_addr_expr) <MEM_REF>: New case. Can you use the TMR_OFFSET macro rather than TREE_OPERAND (op0, 1)? It also seems that you need a stronger check here. Essentially you have to verify that STEP * INDEX + INDEX2 + OFFSET == 0 Right? Jeff >
> Can you use the TMR_OFFSET macro rather than TREE_OPERAND (op0, 1)? > > It also seems that you need a stronger check here. > > Essentially you have to verify that > STEP * INDEX + INDEX2 + OFFSET == 0 > > Right? No, it's MEM_REF, not TARGET_MEM_REF, see build_fold_addr_expr_with_type_loc.
On 10/14/2015 02:57 PM, Eric Botcazou wrote: >> Can you use the TMR_OFFSET macro rather than TREE_OPERAND (op0, 1)? >> >> It also seems that you need a stronger check here. >> >> Essentially you have to verify that >> STEP * INDEX + INDEX2 + OFFSET == 0 >> >> Right? > > No, it's MEM_REF, not TARGET_MEM_REF, see build_fold_addr_expr_with_type_loc. AH, in that case, no objection, patch approved. jeff
On Wed, Oct 14, 2015 at 5:59 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: > Hi, > > this is the regression of ACATS c37213k at -O2 with an upcoming change in > the front-end of the Ada compiler: > > eric@polaris:~/gnat/gnat-head/native> gcc/gnat1 -quiet c37213k.adb -I > /home/eric/gnat/bugs/support -O2 > +===========================GNAT BUG DETECTED==============================+ > | Pro 7.4.0w (20151014-60) (x86_64-suse-linux) GCC error: | > | tree check: expected class 'expression', have | > | 'exceptional' (ssa_name) in tree_operand_check, at tree.h:3431| > | Error detected around c37213k.adb:95:37 Btw, would be really nice to have libbacktrace support for ada ... > It's recompute_tree_invariant_for_addr_expr receiving an SSA_NAME instead of > an ADDR_EXPR when called from gimplify_addr_expr. The sequence is as follows: > we start with this GIMPLE statement: > > *R.43_60 = c37213k.B_1.B_6.proc6.B_4.B_5.value (); [static-chain: &FRAME.60] > [return slot optimization] > > Then IPA clones the function and turns the statement into: > > MEM[(struct c37213k__B_1__B_6__proc6__B_4__nrec *)R.43_46] = > c37213k.B_1.B_6.proc6.B_4.B_5.value (); [static-chain: &FRAME.60] [return slot > optimization] > > The 'value' function has been NRVed and contains: > > .builtin_memcpy (&<retval>, _9, _10); > > and gets inlined so the above statement is rewritten into: > > .builtin_memcpy (&MEM[(struct c37213k__B_1__B_6__proc6__B_4__nrec *)R.43_46], > _174, _175); > > so gimplify_addr_expr is invoked on: > > &MEM[(struct c37213k__B_1__B_6__proc6__B_4__nrec *)R.43_46] > > and gets confused because it doesn't see that it's just R.43_46 (it would have > seen it if the original INDIRECT_REF was still present in lieu of MEM_REF). > > Hence the attached fixlet. Tested on x86_64-suse-linux, OK for the mainline? While the patch looks technically ok I think you'll run into the same issue with a non-zero offset MEM_REF as that will get you a POINTER_PLUS_EXPR from build_fold_addr_expr. We might be lucky not to ICE in recompute_tree_invariant_for_addr_expr because we can access operand zero of that of course. I think recompute_tree_invariant_for_addr_expr misses an assert that it receives an ADDR_EXPR and the gimplify.c caller would need to handle POINTER_PLUS_EXPR specially. Or change your patch to also handle non-zero offset MEM_REFs by simply gimplifying to POINTER_PLUS_EXPR op0, op1. I'd prefer the latter. Thanks, Richard. > > 2015-10-14 Eric Botcazou <ebotcazou@adacore.com> > > * gimplify.c (gimplify_addr_expr) <MEM_REF>: New case. > > -- > Eric Botcazou
Index: gimplify.c =================================================================== --- gimplify.c (revision 228794) +++ gimplify.c (working copy) @@ -4984,6 +4984,12 @@ gimplify_addr_expr (tree *expr_p, gimple ret = GS_OK; break; + case MEM_REF: + if (integer_zerop (TREE_OPERAND (op0, 1))) + goto do_indirect_ref; + + /* ... fall through ... */ + default: /* If we see a call to a declared builtin or see its address being taken (we can unify those cases here) then we can mark