[PATCH] -finline-stringops: don't assume ptr_mode ptr in memset [PR112804]

[PATCH] -finline-stringops: don't assume ptr_mode ptr in memset [PR112804]

[Alexandre Oliva]

On aarch64 -milp32, and presumably on other such targets, ptr can be
in a different mode than ptr_mode in the testcase.  Cope with it.

Regstrapped on x86_64-linux-gnu, also tested the new test on
aarch64-elf.  Ok to install?


for  gcc/ChangeLog

	PR target/112804
	* builtins.cc (try_store_by_multiple_pieces): Use ptr's mode
        for the increment.

for  gcc/testsuite/ChangeLog

	PR target/112804
	* gcc.target/aarch64/inline-mem-set-pr112804.c: New.
---
 gcc/builtins.cc                                    |    2 +-
 .../gcc.target/aarch64/inline-mem-set-pr112804.c   |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/inline-mem-set-pr112804.c

diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index 38b0acff13124..12a535d313f12 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -4519,7 +4519,7 @@ try_store_by_multiple_pieces (rtx to, rtx len, unsigned int ctz_len,
 	  to = change_address (to, QImode, 0);
 	  emit_move_insn (to, val);
 	  if (update_needed)
-	    next_ptr = plus_constant (ptr_mode, ptr, blksize);
+	    next_ptr = plus_constant (GET_MODE (ptr), ptr, blksize);
 	}
       else
 	{
diff --git a/gcc/testsuite/gcc.target/aarch64/inline-mem-set-pr112804.c b/gcc/testsuite/gcc.target/aarch64/inline-mem-set-pr112804.c
new file mode 100644
index 0000000000000..fe8414559864d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/inline-mem-set-pr112804.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-finline-stringops -mabi=ilp32 -ftrivial-auto-var-init=zero" } */
+
+short m(unsigned k) {
+  const unsigned short *n[65];
+  return 0;
+}

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] -finline-stringops: don't assume ptr_mode ptr in memset [PR112804]

[Jeff Law]

On 12/8/23 19:25, Alexandre Oliva wrote:
> 
> On aarch64 -milp32, and presumably on other such targets, ptr can be
> in a different mode than ptr_mode in the testcase.  Cope with it.
> 
> Regstrapped on x86_64-linux-gnu, also tested the new test on
> aarch64-elf.  Ok to install?
> 
> 
> for  gcc/ChangeLog
> 
> 	PR target/112804
> 	* builtins.cc (try_store_by_multiple_pieces): Use ptr's mode
>          for the increment.
> 
> for  gcc/testsuite/ChangeLog
> 
> 	PR target/112804
> 	* gcc.target/aarch64/inline-mem-set-pr112804.c: New.
Hmmm.  I'd like a little more information here I wouldn't expect those 
modes to differ even for ILP32 on a 64bit target.  Are we just papering 
over a problem elsewhere?

Jeff

Re: [PATCH] -finline-stringops: don't assume ptr_mode ptr in memset [PR112804]

[Alexandre Oliva]

On Dec 10, 2023, Jeff Law <jeffreyalaw@gmail.com> wrote:

> On 12/8/23 19:25, Alexandre Oliva wrote:
>> On aarch64 -milp32, and presumably on other such targets, ptr can be
>> in a different mode than ptr_mode in the testcase.  Cope with it.
>> Regstrapped on x86_64-linux-gnu, also tested the new test on
>> aarch64-elf.  Ok to install?
>> 
>> for  gcc/ChangeLog
>> PR target/112804
>> * builtins.cc (try_store_by_multiple_pieces): Use ptr's mode
>> for the increment.
>> for  gcc/testsuite/ChangeLog
>> PR target/112804
>> * gcc.target/aarch64/inline-mem-set-pr112804.c: New.
> Hmmm.  I'd like a little more information here I wouldn't expect those
> modes to differ even for ILP32 on a 64bit target.  Are we just
> papering over a problem elsewhere?

Possibly.  I've run into Pmode/ptr_mode issues before, so I didn't give
it much thought.


Despite ptr_mode's being SImode, and DEST being a 32-bit pointer,
expand_builtin_memset_args gets from get_memory_rtx a (MEM:BLK (REG:DI))

The expansion of dest to RTL is convoluted.  It is first computed in:

(set (reg:DI 102)
    (plus:DI (reg/f:DI 95 virtual-stack-vars)
        (const_int -264 [0xfffffffffffffef8])))

in DImode because virtual-stack-vars is DImode, then it's
SUBREG-narrowed to SImode in expand_expr_addr_expr_1 because of
new_tmode (== pointer_mode), and that's the address stored in addr and
passed to memory_address in get_memory_rtx.

But then something unexpected happens: memory_address_addr_space extends
it back to DImode because address_mode = targetm.addr_space.address_mode
() = Pmode:

(set (reg:DI 103)
    (zero_extend:DI (subreg:SI (reg:DI 102) 0)))

and it's a MEM with (reg:DI 103) as the address that gets passed to
clear_storage_hints, and then try_store_by_multiple_pieces, where the
address is copied from the MEM into ptr, and then, in the update_needed
case I modified in the proposed patch, it gets incremented.


So it looks like my hunch that this was a Pmode/ptr_mode issue was
correct, and I don't see anything particularly fishy in the conversions
above.  Do you?

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] -finline-stringops: don't assume ptr_mode ptr in memset [PR112804]

[Andrew Pinski]

On Sun, Dec 10, 2023 at 2:38 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Dec 10, 2023, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
> > On 12/8/23 19:25, Alexandre Oliva wrote:
> >> On aarch64 -milp32, and presumably on other such targets, ptr can be
> >> in a different mode than ptr_mode in the testcase.  Cope with it.
> >> Regstrapped on x86_64-linux-gnu, also tested the new test on
> >> aarch64-elf.  Ok to install?
> >>
> >> for  gcc/ChangeLog
> >> PR target/112804
> >> * builtins.cc (try_store_by_multiple_pieces): Use ptr's mode
> >> for the increment.
> >> for  gcc/testsuite/ChangeLog
> >> PR target/112804
> >> * gcc.target/aarch64/inline-mem-set-pr112804.c: New.
> > Hmmm.  I'd like a little more information here I wouldn't expect those
> > modes to differ even for ILP32 on a 64bit target.  Are we just
> > papering over a problem elsewhere?
>
> Possibly.  I've run into Pmode/ptr_mode issues before, so I didn't give
> it much thought.
>
>
> Despite ptr_mode's being SImode, and DEST being a 32-bit pointer,
> expand_builtin_memset_args gets from get_memory_rtx a (MEM:BLK (REG:DI))
>
> The expansion of dest to RTL is convoluted.  It is first computed in:
>
> (set (reg:DI 102)
>     (plus:DI (reg/f:DI 95 virtual-stack-vars)
>         (const_int -264 [0xfffffffffffffef8])))
>
> in DImode because virtual-stack-vars is DImode, then it's
> SUBREG-narrowed to SImode in expand_expr_addr_expr_1 because of
> new_tmode (== pointer_mode), and that's the address stored in addr and
> passed to memory_address in get_memory_rtx.
>
> But then something unexpected happens: memory_address_addr_space extends
> it back to DImode because address_mode = targetm.addr_space.address_mode
> () = Pmode:
>
> (set (reg:DI 103)
>     (zero_extend:DI (subreg:SI (reg:DI 102) 0)))
>
> and it's a MEM with (reg:DI 103) as the address that gets passed to
> clear_storage_hints, and then try_store_by_multiple_pieces, where the
> address is copied from the MEM into ptr, and then, in the update_needed
> case I modified in the proposed patch, it gets incremented.
>
>
> So it looks like my hunch that this was a Pmode/ptr_mode issue was
> correct, and I don't see anything particularly fishy in the conversions
> above.  Do you?

That looks correct.
Just for info; AARCH64:ILP32, even though the exposed pointer size is
32bit, the address for the loads is always done as 64bit.
That is why there is a difference in Pmode and ptr_mode here.
So Pmode still needs to be 64bit due to the addres requirement while
ptr_mode is 32bit.

x32 on x86_64 implements 2 different modes, one where Pmode==ptr_mode
and one where they differ. The default for x32 is Pmode==ptr_mode
IIRC.  This is because x86 has load instructions which handles
addresses that are 32bit (zero-extended IIRC).
IA64 HPUX has a similar mode to AARCH64:ILP32 but I doubt anyone tests
that any more or even supported; it is also where most of the original
testing for Pmode!=ptr_mode happened.
PowerPC64 has a mode where Pmode==ptr_mode==SImode but
word_mode==DImode; this kinda works on Linux but if you hit a place
into the kernel only the lower 32bits is save; it does work fully on
powerpc Darwin (Mac OS X) though.

Thanks,
Andrew Pinski

>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] -finline-stringops: don't assume ptr_mode ptr in memset [PR112804]

[Jeff Law]

On 12/10/23 15:38, Alexandre Oliva wrote:
> On Dec 10, 2023, Jeff Law <jeffreyalaw@gmail.com> wrote:
> 
>> On 12/8/23 19:25, Alexandre Oliva wrote:
>>> On aarch64 -milp32, and presumably on other such targets, ptr can be
>>> in a different mode than ptr_mode in the testcase.  Cope with it.
>>> Regstrapped on x86_64-linux-gnu, also tested the new test on
>>> aarch64-elf.  Ok to install?
>>>
>>> for  gcc/ChangeLog
>>> PR target/112804
>>> * builtins.cc (try_store_by_multiple_pieces): Use ptr's mode
>>> for the increment.
>>> for  gcc/testsuite/ChangeLog
>>> PR target/112804
>>> * gcc.target/aarch64/inline-mem-set-pr112804.c: New.
>> Hmmm.  I'd like a little more information here I wouldn't expect those
>> modes to differ even for ILP32 on a 64bit target.  Are we just
>> papering over a problem elsewhere?
> 
> Possibly.  I've run into Pmode/ptr_mode issues before, so I didn't give
> it much thought.
I'm not very familiar with ptr_mode.

I do remember that Pmode can ultimately differ from the mode that you'd 
get from turning POINTER_SIZE into an integer mode.  For example we 
might have Pmode defined as PSImode while POINTER_SIZE is 32.  That was 
the model used on the mn102 as pointers were quite literally 24 bits in 
a register.

> 
> 
> Despite ptr_mode's being SImode, and DEST being a 32-bit pointer,
> expand_builtin_memset_args gets from get_memory_rtx a (MEM:BLK (REG:DI))
> 
> The expansion of dest to RTL is convoluted.  It is first computed in:
> 
> (set (reg:DI 102)
>      (plus:DI (reg/f:DI 95 virtual-stack-vars)
>          (const_int -264 [0xfffffffffffffef8])))
> 
> in DImode because virtual-stack-vars is DImode, then it's
> SUBREG-narrowed to SImode in expand_expr_addr_expr_1 because of
> new_tmode (== pointer_mode), and that's the address stored in addr and
> passed to memory_address in get_memory_rtx.
> 
> But then something unexpected happens: memory_address_addr_space extends
> it back to DImode because address_mode = targetm.addr_space.address_mode
> () = Pmode:
> 
> (set (reg:DI 103)
>      (zero_extend:DI (subreg:SI (reg:DI 102) 0)))
> 
> and it's a MEM with (reg:DI 103) as the address that gets passed to
> clear_storage_hints, and then try_store_by_multiple_pieces, where the
> address is copied from the MEM into ptr, and then, in the update_needed
> case I modified in the proposed patch, it gets incremented.
> 
> 
> So it looks like my hunch that this was a Pmode/ptr_mode issue was
> correct, and I don't see anything particularly fishy in the conversions
> above.  Do you?
Not terribly fishy.  Mostly I think I just wasn't aware of ptr_mode.  It 
looks very similar to some of the kinds of bugs we found on the mn102 
way back in the day and since then we've grown some infrastructure to 
handle it better.

OK for the trunk. Thanks for walking me through it.

jeff
>