Re: [PATCH 02/25] Propagate address spaces to builtins.

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

On 9/4/19 3:21 PM, Kyrill Tkachov wrote:
>
> On 9/3/19 4:00 PM, Jeff Law wrote:
> > On 9/3/19 8:01 AM, Kyrill Tkachov wrote:
> >> Hi all,
> >>
> >> On 9/5/18 12:48 PM, ams@codesourcery.com wrote:
> >>> At present, pointers passed to builtin functions, including atomic
> >>> operators,
> >>> are stripped of their address space properties.  This doesn't seem 
> to be
> >>> deliberate, it just omits to copy them.
> >>>
> >>> Not only that, but it forces pointer sizes to Pmode, which isn't
> >>> appropriate
> >>> for all address spaces.
> >>>
> >>> This patch attempts to correct both issues.  It works for GCN 
> atomics and
> >>> GCN OpenACC gang-private variables.
> >>>
> >>> 2018-09-05  Andrew Stubbs <ams@codesourcery.com>
> >>>              Julian Brown <julian@codesourcery.com>
> >>>
> >>>          gcc/
> >>>          * builtins.c (get_builtin_sync_mem): Handle address spaces.
> >>
> >> Sorry for responding to this so late. I'm testing a rebased version of
> >> Richard's OOL atomic patches [1] and am hitting an ICE building the
> >> -mabi=ilp32 libgfortran multilib for aarch64-none-elf:
> >>
> >> 0x7284db emit_library_call_value_1(int, rtx_def*, rtx_def*,
> >> libcall_type, machine_mode, int, std::pair<rtx_def*, machine_mode>*)
> >>          $SRC/gcc/calls.c:4915
> >> 0x1037817 emit_library_call_value(rtx_def*, rtx_def*, libcall_type,
> >> machine_mode, rtx_def*, machine_mode, rtx_def*, machine_mode, rtx_def*,
> >> machine_mode)
> >>          $SRC/gcc/rtl.h:4240
> >> 0x1037817 aarch64_expand_compare_and_swap(rtx_def**)
> >>          $SRC/gcc/config/aarch64/aarch64.c:16981
> >> 0x1353a43 gen_atomic_compare_and_swapsi(rtx_def*, rtx_def*, rtx_def*,
> >> rtx_def*, rtx_def*, rtx_def*, rtx_def*, rtx_def*)
> >>          $SRC/gcc/config/aarch64/atomics.md:34
> >> 0xb1f9f1 insn_gen_fn::operator()(rtx_def*, rtx_def*, rtx_def*, 
> rtx_def*,
> >> rtx_def*, rtx_def*, rtx_def*, rtx_def*) const
> >>          $SRC/gcc/recog.h:324
> >> 0xb1f9f1 maybe_gen_insn(insn_code, unsigned int, expand_operand*)
> >>          $SRC/gcc/optabs.c:7443
> >> 0xb1fa78 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
> >>          $SRC/gcc/optabs.c:7459
> >> 0xb21024 expand_atomic_compare_and_swap(rtx_def**, rtx_def**, rtx_def*,
> >> rtx_def*, rtx_def*, bool, memmodel, memmodel)
> >>          $SRC/gcc/optabs.c:6448
> >> 0x709bd3 expand_builtin_atomic_compare_exchange
> >>          $SRC/gcc/builtins.c:6379
> >> 0x71a4e9 expand_builtin(tree_node*, rtx_def*, rtx_def*, 
> machine_mode, int)
> >>          $SRC/gcc/builtins.c:8147
> >> 0x88b746 expand_expr_real_1(tree_node*, rtx_def*, machine_mode,
> >> expand_modifier, rtx_def**, bool)
> >>          $SRC/gcc/expr.c:11052
> >> 0x88cce6 expand_expr_real(tree_node*, rtx_def*, machine_mode,
> >> expand_modifier, rtx_def**, bool)
> >>          $SRC/gcc/expr.c:8289
> >> 0x74cb47 expand_expr
> >>          $SRC/gcc/expr.h:281
> >> 0x74cb47 expand_call_stmt
> >>          $SRC/gcc/cfgexpand.c:2731
> >> 0x74cb47 expand_gimple_stmt_1
> >>          $SRC/gcc/cfgexpand.c:3710
> >> 0x74cb47 expand_gimple_stmt
> >>          $SRC/gcc/cfgexpand.c:3875
> >> 0x75439b expand_gimple_basic_block
> >>          $SRC/gcc/cfgexpand.c:5915
> >> 0x7563ab execute
> >>          $SRC/gcc/cfgexpand.c:6538
> >> Please submit a full bug report,
> >> with preprocessed source if appropriate.
> >> Please include the complete backtrace with any bug report.
> >> See <https://gcc.gnu.org/bugs/> for instructions.
> >>
> >> A MEM rtx now uses a DImode address where for ILP32 we expect SImode.
> >>
> >> This looks to be because....
> >>
> >> [1] https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00062.html
> >>
> >>
> >>> ---
> >>>   gcc/builtins.c | 13 ++++++++++---
> >>>   1 file changed, 10 insertions(+), 3 deletions(-)
> >>>
> >> 0002-Propagate-address-spaces-to-builtins.patch
> >>
> >> diff --git a/gcc/builtins.c b/gcc/builtins.c
> >> index 58ea747..361361c 100644
> >> --- a/gcc/builtins.c
> >> +++ b/gcc/builtins.c
> >> @@ -5781,14 +5781,21 @@ static rtx
> >>   get_builtin_sync_mem (tree loc, machine_mode mode)
> >>   {
> >>     rtx addr, mem;
> >> +  int addr_space = TYPE_ADDR_SPACE (POINTER_TYPE_P (TREE_TYPE (loc))
> >> +                    ? TREE_TYPE (TREE_TYPE (loc))
> >> +                    : TREE_TYPE (loc));
> >> +  scalar_int_mode addr_mode = targetm.addr_space.address_mode
> >> (addr_space);
> >>
> >> ... This now returns Pmode (the default for the hook) for aarch64 
> ILP32,
> >> which is always DImode.
> >>
> >> -  addr = expand_expr (loc, NULL_RTX, ptr_mode, EXPAND_SUM);
> >>
> >> Before this patch we used ptr_mode, which does the right thing for
> >> AArch64 ILP32.
> >> Do you think we should just be implementing
> >> targetm.addr_space.address_mode for AArch64 to return SImode for ILP32?
> > Possibly.   Is there any fallout from making that change?
>
> Unfortunately some ICEs when building libgcc with POST_INC arguments
> output :(
>
> I'll need to dig further.

Adding a convert_memory_address to ptr_mode before each call to 
emit_library_call_value in the OOL atomics code to convert the addresses 
does fix it the ICEs.

Let's see what testing shows.

Kyrill



>
> Thanks,
>
> Kyrill
>
>
> >
> > Jeff