This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]