[02/25] Propagate address spaces to builtins.

Message ID c33c97c491ef8718f23f3282ae5caafbe141df10.1536144068.git.ams@codesourcery.com
State New
Headers show
Series
  • AMD GCN Port
Related show

Commit Message

Andrew Stubbs Sept. 5, 2018, 11:48 a.m.
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.
---
 gcc/builtins.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Richard Biener Sept. 20, 2018, 12:59 p.m. | #1
On Wed, Sep 5, 2018 at 1:50 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.


OK.

Richard.

> 2018-09-05  Andrew Stubbs  <ams@codesourcery.com>

>             Julian Brown  <julian@codesourcery.com>

>

>         gcc/

>         * builtins.c (get_builtin_sync_mem): Handle address spaces.

> ---

>  gcc/builtins.c | 13 ++++++++++---

>  1 file changed, 10 insertions(+), 3 deletions(-)

>
Andreas Schwab Sept. 22, 2018, 6:51 p.m. | #2
On Sep 05 2018, <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.


That breaks aarch64 ILP32.

../../../../libgomp/ordered.c: In function 'GOMP_doacross_wait':
../../../../libgomp/ordered.c:486:1: internal compiler error: output_operand: invalid address mode
486 | }
    | ^
0xf7219f aarch64_print_address_internal
        ../../gcc/config/aarch64/aarch64.c:7163
0xf7269b aarch64_print_operand_address
        ../../gcc/config/aarch64/aarch64.c:7267
0x8871ef output_address(machine_mode, rtx_def*)
        ../../gcc/final.c:4069
0xf7302b aarch64_print_operand
        ../../gcc/config/aarch64/aarch64.c:6952
0x887133 output_operand(rtx_def*, int)
        ../../gcc/final.c:4053
0x887b5b output_asm_insn(char const*, rtx_def**)
        ../../gcc/final.c:3965
0x889157 output_asm_insn(char const*, rtx_def**)
        ../../gcc/final.c:3842
0x889157 final_scan_insn_1
        ../../gcc/final.c:3103
0x88984b final_scan_insn(rtx_insn*, _IO_FILE*, int, int, int*)
        ../../gcc/final.c:3149
0x889b1f final_1
        ../../gcc/final.c:2019
0x88aaff rest_of_handle_final
        ../../gcc/final.c:4660
0x88aaff execute
        ../../gcc/final.c:4734

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Andrew Stubbs Sept. 24, 2018, 4:52 p.m. | #3
On 22/09/18 19:51, Andreas Schwab wrote:
> That breaks aarch64 ILP32.


I'm struggling to reproduce this because apparently I don't know how to 
build aarch64 ILP32.

Presumably, in order to be building libgomp this must be an 
aarch64-linux-gnu toolchain, but when I set --with-abi=ilp32 I can't 
build glibc:

In file included from ../nptl/descr.h:24, 
 
 

                  from ../sysdeps/aarch64/nptl/tls.h:44, 
 
 

                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:29, 
 
 

                  from <stdin>:1: 
 
 

../include/setjmp.h:50:3: error: static assertion failed: "offset of 
__saved_mask field of struct __jmp_buf_tag != 184

What should I have done?

Thanks

Andrew
Andreas Schwab Sept. 24, 2018, 5:33 p.m. | #4
On Sep 24 2018, Andrew Stubbs <ams@codesourcery.com> wrote:

> What should I have done?


Make sure you have the ILP32 patches for glibc and kernel.  You can get
them from the arm/ilp32 branch on sourceware.org
<http://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/arm/ilp32>
and the staging/ilp32-4.17 branch of the arm64 kernel tree
<https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=staging/ilp32-4.17>.

You can also get pre-built packages from
<https://download.opensuse.org/repositories/devel:/ARM:/Factory:/Contrib:/ILP32/standard/>

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Kyrill Tkachov Sept. 3, 2019, 2:01 p.m. | #5
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?

Thanks,
Kyrill


-  addr = convert_memory_address (Pmode, addr);
+  addr = expand_expr (loc, NULL_RTX, addr_mode, EXPAND_SUM);
  
    /* Note that we explicitly do not want any alias information for this
       memory, so that we kill all other live memories.  Otherwise we don't
       satisfy the full barrier semantics of the intrinsic.  */
-  mem = validize_mem (gen_rtx_MEM (mode, addr));
+  mem = gen_rtx_MEM (mode, addr);
+
+  set_mem_addr_space (mem, addr_space);
+
+  mem = validize_mem (mem);
  
    /* The alignment needs to be at least according to that of the mode.  */
    set_mem_align (mem, MAX (GET_MODE_ALIGNMENT (mode),
Jeff Law Sept. 3, 2019, 3 p.m. | #6
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?

Jeff
Andrew Stubbs Sept. 3, 2019, 3:43 p.m. | #7
On 03/09/2019 15:01, Kyrill Tkachov wrote:
> 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:


I thought Andreas already fixed ILP32.

https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01439.html

Andrew
Kyrill Tkachov Sept. 4, 2019, 2:21 p.m. | #8
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.

Thanks,

Kyrill


>

> Jeff
Kyrill Tkachov Sept. 4, 2019, 3:29 p.m. | #9
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

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);
 
-  addr = expand_expr (loc, NULL_RTX, ptr_mode, EXPAND_SUM);
-  addr = convert_memory_address (Pmode, addr);
+  addr = expand_expr (loc, NULL_RTX, addr_mode, EXPAND_SUM);
 
   /* Note that we explicitly do not want any alias information for this
      memory, so that we kill all other live memories.  Otherwise we don't
      satisfy the full barrier semantics of the intrinsic.  */
-  mem = validize_mem (gen_rtx_MEM (mode, addr));
+  mem = gen_rtx_MEM (mode, addr);
+
+  set_mem_addr_space (mem, addr_space);
+
+  mem = validize_mem (mem);
 
   /* The alignment needs to be at least according to that of the mode.  */
   set_mem_align (mem, MAX (GET_MODE_ALIGNMENT (mode),