On 9/3/19 8:01 AM, Kyrill Tkachov wrote: > Hi all, > > On 9/5/18 12:48 PM, a...@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 <a...@codesourcery.com> >> Julian Brown <jul...@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?
Jeff
