On Thu, 8 Oct 2015, Jan Hubicka wrote:
> > > - STRIP_NOPS (arg0);
> > > - STRIP_NOPS (arg1);
> > > + STRIP_NOPS (arg0);
> > > + STRIP_NOPS (arg1);
> > > + }
> > > + else
> > > + /* Addresses of NOP_EXPR (and many other things) are not well
> > > defined.
> > > + Check that we did not forget to drop the
> > > + OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags. */
> > > + gcc_checking_assert (TREE_CODE (arg0) != NOP_EXPR
> > > + && TREE_CODE (arg1) != NOP_EXPR);
> >
> > Use ! CONVERT_EXPR_P (arg0) && ! CONVERT_EXPR_P (arg1)
>
> Hi,
> the x86_64 testing actually shows one regression at
> gfortran.dg/coarray/coindexed_1.f90. The problem is the new addrt that we do
> not take an address of NOP_EXPR. Fortran indeed does that:
>
> <addr_expr 0x7ffff6caa2e0
> type <pointer_type 0x7ffff6c9fdc8
> type <array_type 0x7ffff6c9fb28 type <integer_type 0x7ffff6adb540
> character(kind=1)>
> string-flag BLK
> size <integer_cst 0x7ffff6c917b0 constant 56>
> unit size <integer_cst 0x7ffff6c91780 constant 7>
> align 8 symtab 0 alias set -1 canonical type 0x7ffff6c9fb28
> domain <integer_type 0x7ffff6c9fa80>
> pointer_to_this <pointer_type 0x7ffff6c9fdc8>>
> unsigned DI
> size <integer_cst 0x7ffff6ad7bd0 constant 64>
> unit size <integer_cst 0x7ffff6ad7be8 constant 8>
> align 64 symtab 0 alias set -1 canonical type 0x7ffff6c9fdc8>
>
> arg 0 <nop_expr 0x7ffff6caa2a0 type <array_type 0x7ffff6c9fb28>
>
> arg 0 <var_decl 0x7ffff6ae1a20 str2a type <array_type 0x7ffff6c9fbd0>
> used static decl_0 BLK file
> /aux/hubicka/trunk-9/gcc/testsuite/gfortran.dg/coarray/coindexed_1.f90 line
> 10 col 0 size <integer_cst 0x7ffff6c917b0 56> unit size <integer_cst
> 0x7ffff6c91780 7>
> align 8 context <function_decl 0x7ffff6c97a80 char_test> chain
> <var_decl 0x7ffff6ae1990 str1b>>>
>
>
> A NOP_EXPR truning one array type to another seems somewhat suspicious.
> Perhaps it should be VIEW_CONVERT_EXPR? I tracked down that the NOP_EXPR is
> introduced by:
>
> #3 0x000000000068c800 in gfc_conv_array_ref (se=se@entry=0x7fffffffe320,
> ar=ar@entry=0x1db8e78, expr=expr@entry=0x1db8da0, where=where@entry=0x1db8df0)
> at ../../gcc/fortran/trans-array.c:3299
> 3299 se->expr = fold_convert (TYPE_MAIN_VARIANT (TREE_TYPE
> (se->expr)),
> (gdb) l
> 3294 && TREE_CODE (TREE_TYPE (se->expr)) == POINTER_TYPE)
> 3295 se->expr = build_fold_indirect_ref_loc (input_location,
> se->expr);
> 3296
> 3297 /* Use the actual tree type and not the wrapped coarray. */
> 3298 if (!se->want_pointer)
> 3299 se->expr = fold_convert (TYPE_MAIN_VARIANT (TREE_TYPE
> (se->expr)),
> 3300 se->expr);
> 3301 }
>
> So it seems to be fuly concious decision to add the conversion.
> I suppose this may make sense to the frontend trees. It seems resonable to
> however drop the NOP_EXPR before building ADDR_EXPR as follows. Calling
> get_base_address on something that contains a NOP_EXPR in address is also
> a bad idea.
>
> Bootstrapping/regtesting x86_64-linux, seems sane?
Hmm, I'd rather not use NOP_EXPR on aggregate types. I also see no reason
to have NOPs to a main variant. It's btw the only NOP we allow on
aggregates in fold_convert...
Using a VIEW_CONVERT_EXPR would be cleaner IMHO.
Of course your patch also works and I'll leave the decision to the
frontend maintainers.
Thanks,
Richard.
> Honza
>
>
> * trans.c (gfc_build_addr_expr): Do not build ADDR_EXPR of NOP_EXPR.
> Index: trans.c
> ===================================================================
> --- trans.c (revision 228582)
> +++ trans.c (working copy)
> @@ -297,6 +297,7 @@ gfc_build_addr_expr (tree type, tree t)
> }
> else
> {
> + STRIP_NOPS (t);
> tree base = get_base_address (t);
> if (base && DECL_P (base))
> TREE_ADDRESSABLE (base) = 1;
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
