* [RS6000] powerpc64 -mcmodel=medium large symbol offsets
@ 2013-09-06 7:13 Alan Modra
2013-09-06 18:18 ` David Edelsohn
0 siblings, 1 reply; 8+ messages in thread
From: Alan Modra @ 2013-09-06 7:13 UTC (permalink / raw)
To: David Edelsohn, gcc-patches
The following testcase taken from the linux kernel is miscompiled on
powerpc64-linux.
/* -m64 -mcmodel=medium -O -S -fno-section-anchors */
static int x;
unsigned long
foo (void)
{
return ((unsigned long) &x) - 0xc000000000000000;
}
generates
addis 3,2,x+4611686018427387904@toc@ha
addi 3,3,x+4611686018427387904@toc@l
blr
losing the top 32 bits of the offset. Sadly, the assembler and linker
do not complain, which is a hole in the ABI. (@ha and _HA relocs as
per the ABI won't complain about overflow since they might be used in
a @highesta, @highera sequence loading a 64-bit value.)
This patch stops combine merging large offsets into a symbol addend
by copying code from reg_or_add_cint_operand to a new predicate,
add_cint_operand, and using that to restrict the range of offsets.
Bootstrapped and regression tested powerpc64-linux. OK to apply?
* config/rs6000/predicates.md (add_cint_operand): New.
* config/rs6000/rs6000.md (largetoc_high_plus): Restrict offset
using add_cint_operand.
(largetoc_high_plus_aix): Likewise.
Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md (revision 202264)
+++ gcc/config/rs6000/predicates.md (working copy)
@@ -376,6 +376,12 @@
(ior (match_code "const_int")
(match_operand 0 "gpc_reg_operand")))
+;; Return 1 if op is a constant integer valid for addition with addis, addi.
+(define_predicate "add_cint_operand"
+ (and (match_code "const_int")
+ (match_test "(unsigned HOST_WIDE_INT) (INTVAL (op) + 0x80008000)
+ < (unsigned HOST_WIDE_INT) 0x100000000ll")))
+
;; Return 1 if op is a constant integer valid for addition
;; or non-special register.
(define_predicate "reg_or_add_cint_operand"
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md (revision 202264)
+++ gcc/config/rs6000/rs6000.md (working copy)
@@ -12207,7 +12209,7 @@
(unspec [(match_operand:DI 1 "" "")
(match_operand:DI 2 "gpc_reg_operand" "b")]
UNSPEC_TOCREL)
- (match_operand 3 "const_int_operand" "n"))))]
+ (match_operand 3 "add_cint_operand" "n"))))]
"TARGET_ELF && TARGET_CMODEL != CMODEL_SMALL"
"addis %0,%2,%1+%3@toc@ha")
@@ -12218,7 +12220,7 @@
(unspec [(match_operand:P 1 "" "")
(match_operand:P 2 "gpc_reg_operand" "b")]
UNSPEC_TOCREL)
- (match_operand 3 "const_int_operand" "n"))))]
+ (match_operand 3 "add_cint_operand" "n"))))]
"TARGET_XCOFF && TARGET_CMODEL != CMODEL_SMALL"
"addis %0,%1+%3@u(%2)")
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RS6000] powerpc64 -mcmodel=medium large symbol offsets
2013-09-06 7:13 [RS6000] powerpc64 -mcmodel=medium large symbol offsets Alan Modra
@ 2013-09-06 18:18 ` David Edelsohn
2013-09-06 23:36 ` Alan Modra
0 siblings, 1 reply; 8+ messages in thread
From: David Edelsohn @ 2013-09-06 18:18 UTC (permalink / raw)
To: Alan Modra, GCC Patches
On Fri, Sep 6, 2013 at 3:13 AM, Alan Modra <amodra@gmail.com> wrote:
> The following testcase taken from the linux kernel is miscompiled on
> powerpc64-linux.
>
> /* -m64 -mcmodel=medium -O -S -fno-section-anchors */
> static int x;
>
> unsigned long
> foo (void)
> {
> return ((unsigned long) &x) - 0xc000000000000000;
> }
>
> generates
> addis 3,2,x+4611686018427387904@toc@ha
> addi 3,3,x+4611686018427387904@toc@l
> blr
>
> losing the top 32 bits of the offset. Sadly, the assembler and linker
> do not complain, which is a hole in the ABI. (@ha and _HA relocs as
> per the ABI won't complain about overflow since they might be used in
> a @highesta, @highera sequence loading a 64-bit value.)
>
> This patch stops combine merging large offsets into a symbol addend
> by copying code from reg_or_add_cint_operand to a new predicate,
> add_cint_operand, and using that to restrict the range of offsets.
> Bootstrapped and regression tested powerpc64-linux. OK to apply?
>
> * config/rs6000/predicates.md (add_cint_operand): New.
> * config/rs6000/rs6000.md (largetoc_high_plus): Restrict offset
> using add_cint_operand.
> (largetoc_high_plus_aix): Likewise.
This patch should include a testcase.
But what user feedback are you expecting if the offset is too large,
such as your example? In my test with the patch, it produces an
unrecognizable insn error, which seems less than friendly.
Thanks, David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RS6000] powerpc64 -mcmodel=medium large symbol offsets
2013-09-06 18:18 ` David Edelsohn
@ 2013-09-06 23:36 ` Alan Modra
2013-09-07 8:03 ` Alan Modra
0 siblings, 1 reply; 8+ messages in thread
From: Alan Modra @ 2013-09-06 23:36 UTC (permalink / raw)
To: David Edelsohn; +Cc: GCC Patches
On Fri, Sep 06, 2013 at 02:18:49PM -0400, David Edelsohn wrote:
> On Fri, Sep 6, 2013 at 3:13 AM, Alan Modra <amodra@gmail.com> wrote:
> > The following testcase taken from the linux kernel is miscompiled on
> > powerpc64-linux.
> >
> > /* -m64 -mcmodel=medium -O -S -fno-section-anchors */
> > static int x;
> >
> > unsigned long
> > foo (void)
> > {
> > return ((unsigned long) &x) - 0xc000000000000000;
> > }
> >
> > generates
> > addis 3,2,x+4611686018427387904@toc@ha
> > addi 3,3,x+4611686018427387904@toc@l
> > blr
> >
> > losing the top 32 bits of the offset. Sadly, the assembler and linker
> > do not complain, which is a hole in the ABI. (@ha and _HA relocs as
> > per the ABI won't complain about overflow since they might be used in
> > a @highesta, @highera sequence loading a 64-bit value.)
> >
> > This patch stops combine merging large offsets into a symbol addend
> > by copying code from reg_or_add_cint_operand to a new predicate,
> > add_cint_operand, and using that to restrict the range of offsets.
> > Bootstrapped and regression tested powerpc64-linux. OK to apply?
> >
> > * config/rs6000/predicates.md (add_cint_operand): New.
> > * config/rs6000/rs6000.md (largetoc_high_plus): Restrict offset
> > using add_cint_operand.
> > (largetoc_high_plus_aix): Likewise.
>
> This patch should include a testcase.
>
> But what user feedback are you expecting if the offset is too large,
> such as your example? In my test with the patch, it produces an
> unrecognizable insn error, which seems less than friendly.
The testcase gives me
.L.foo:
lis 9,0x4000
sldi 9,9,32
addis 3,2,x@toc@ha
addi 3,3,x@toc@l
add 3,3,9
blr
How did you manage to get an unrecognizable insn? I can't see how we
generate the pattern except in combine.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RS6000] powerpc64 -mcmodel=medium large symbol offsets
2013-09-06 23:36 ` Alan Modra
@ 2013-09-07 8:03 ` Alan Modra
2013-09-09 9:13 ` Alan Modra
0 siblings, 1 reply; 8+ messages in thread
From: Alan Modra @ 2013-09-07 8:03 UTC (permalink / raw)
To: David Edelsohn, GCC Patches
On Sat, Sep 07, 2013 at 09:06:08AM +0930, Alan Modra wrote:
> The testcase gives me
>
> .L.foo:
> lis 9,0x4000
> sldi 9,9,32
> addis 3,2,x@toc@ha
> addi 3,3,x@toc@l
> add 3,3,9
> blr
>
> How did you manage to get an unrecognizable insn? I can't see how we
> generate the pattern except in combine.
Never mind. I updated and rebuilt from a clean tree and now see the
failure too. "tocrefdi" is where combine is still munging together
the large offset.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RS6000] powerpc64 -mcmodel=medium large symbol offsets
2013-09-07 8:03 ` Alan Modra
@ 2013-09-09 9:13 ` Alan Modra
2013-09-09 15:21 ` David Edelsohn
2013-09-11 11:41 ` Alan Modra
0 siblings, 2 replies; 8+ messages in thread
From: Alan Modra @ 2013-09-09 9:13 UTC (permalink / raw)
To: David Edelsohn, GCC Patches
Revised patch with testcase. This one also fixes a small problem with
reg_or_add_cint_operand in that any 32-bit value is valid for SImode.
Compare with reg_or_sub_cint_operand.
Bootstrapped and regression tested powerpc64-linux. OK to apply?
gcc/
* config/rs6000/predicates.md (add_cint_operand): New.
(reg_or_add_cint_operand): Use add_cint_operand.
* config/rs6000/rs6000.md (largetoc_high_plus): Restrict offset
using add_cint_operand.
(largetoc_high_plus_aix, small_toc_ref): Likewise.
gcc/testsuite/
* gcc.target/powerpc/medium_offset.c: New.
Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md (revision 202351)
+++ gcc/config/rs6000/predicates.md (working copy)
@@ -376,12 +376,18 @@
(ior (match_code "const_int")
(match_operand 0 "gpc_reg_operand")))
+;; Return 1 if op is a constant integer valid for addition with addis, addi.
+(define_predicate "add_cint_operand"
+ (and (match_code "const_int")
+ (match_test "(unsigned HOST_WIDE_INT)
+ (INTVAL (op) + (mode == SImode ? 0x80000000 : 0x80008000))
+ < (unsigned HOST_WIDE_INT) 0x100000000ll")))
+
;; Return 1 if op is a constant integer valid for addition
;; or non-special register.
(define_predicate "reg_or_add_cint_operand"
(if_then_else (match_code "const_int")
- (match_test "(unsigned HOST_WIDE_INT) (INTVAL (op) + 0x80008000)
- < (unsigned HOST_WIDE_INT) 0x100000000ll")
+ (match_operand 0 "add_cint_operand")
(match_operand 0 "gpc_reg_operand")))
;; Return 1 if op is a constant integer valid for subtraction
@@ -1697,7 +1703,7 @@
(define_predicate "small_toc_ref"
(match_code "unspec,plus")
{
- if (GET_CODE (op) == PLUS && CONST_INT_P (XEXP (op, 1)))
+ if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), mode))
op = XEXP (op, 0);
return GET_CODE (op) == UNSPEC && XINT (op, 1) == UNSPEC_TOCREL;
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md (revision 202351)
+++ gcc/config/rs6000/rs6000.md (working copy)
@@ -12207,7 +12209,7 @@
(unspec [(match_operand:DI 1 "" "")
(match_operand:DI 2 "gpc_reg_operand" "b")]
UNSPEC_TOCREL)
- (match_operand 3 "const_int_operand" "n"))))]
+ (match_operand:DI 3 "add_cint_operand" "n"))))]
"TARGET_ELF && TARGET_CMODEL != CMODEL_SMALL"
"addis %0,%2,%1+%3@toc@ha")
@@ -12218,7 +12220,7 @@
(unspec [(match_operand:P 1 "" "")
(match_operand:P 2 "gpc_reg_operand" "b")]
UNSPEC_TOCREL)
- (match_operand 3 "const_int_operand" "n"))))]
+ (match_operand:P 3 "add_cint_operand" "n"))))]
"TARGET_XCOFF && TARGET_CMODEL != CMODEL_SMALL"
"addis %0,%1+%3@u(%2)")
Index: gcc/testsuite/gcc.target/powerpc/medium_offset.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/medium_offset.c (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/medium_offset.c (revision 0)
@@ -0,0 +1,12 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-require-effective-target lp64 } */
+/* { dg-options "-O" } */
+/* { dg-final { scan-assembler-not "\\+4611686018427387904" } } */
+
+static int x;
+
+unsigned long
+foo (void)
+{
+ return ((unsigned long) &x) - 0xc000000000000000;
+}
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RS6000] powerpc64 -mcmodel=medium large symbol offsets
2013-09-09 9:13 ` Alan Modra
@ 2013-09-09 15:21 ` David Edelsohn
2013-09-11 11:41 ` Alan Modra
1 sibling, 0 replies; 8+ messages in thread
From: David Edelsohn @ 2013-09-09 15:21 UTC (permalink / raw)
To: David Edelsohn, GCC Patches
On Mon, Sep 9, 2013 at 5:07 AM, Alan Modra <amodra@gmail.com> wrote:
> Revised patch with testcase. This one also fixes a small problem with
> reg_or_add_cint_operand in that any 32-bit value is valid for SImode.
> Compare with reg_or_sub_cint_operand.
>
> Bootstrapped and regression tested powerpc64-linux. OK to apply?
>
> gcc/
> * config/rs6000/predicates.md (add_cint_operand): New.
> (reg_or_add_cint_operand): Use add_cint_operand.
> * config/rs6000/rs6000.md (largetoc_high_plus): Restrict offset
> using add_cint_operand.
> (largetoc_high_plus_aix, small_toc_ref): Likewise.
> gcc/testsuite/
> * gcc.target/powerpc/medium_offset.c: New.
Okay. This seems much better.
Thanks, David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RS6000] powerpc64 -mcmodel=medium large symbol offsets
2013-09-09 9:13 ` Alan Modra
2013-09-09 15:21 ` David Edelsohn
@ 2013-09-11 11:41 ` Alan Modra
2013-09-11 21:48 ` David Edelsohn
1 sibling, 1 reply; 8+ messages in thread
From: Alan Modra @ 2013-09-11 11:41 UTC (permalink / raw)
To: David Edelsohn, GCC Patches
On Mon, Sep 09, 2013 at 06:37:03PM +0930, Alan Modra wrote:
> gcc/
> * config/rs6000/predicates.md (add_cint_operand): New.
> (reg_or_add_cint_operand): Use add_cint_operand.
> * config/rs6000/rs6000.md (largetoc_high_plus): Restrict offset
> using add_cint_operand.
> (largetoc_high_plus_aix, small_toc_ref): Likewise.
> gcc/testsuite/
> * gcc.target/powerpc/medium_offset.c: New.
I missed seeing one testcase regression caused by this patch. :-(
gcc.c-torture/compile/pr41634.c at -O3 gets an "insn does not satisfy
its constraints". Fixed with the following. OK to apply?
* config/rs6000/rs6000.c (toc_relative_expr_p): Use add_cint_operand.
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 202428)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -5926,7 +5906,7 @@ toc_relative_expr_p (const_rtx op, bool strict)
tocrel_base = op;
tocrel_offset = const0_rtx;
- if (GET_CODE (op) == PLUS && CONST_INT_P (XEXP (op, 1)))
+ if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), GET_MODE (op)))
{
tocrel_base = XEXP (op, 0);
tocrel_offset = XEXP (op, 1);
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RS6000] powerpc64 -mcmodel=medium large symbol offsets
2013-09-11 11:41 ` Alan Modra
@ 2013-09-11 21:48 ` David Edelsohn
0 siblings, 0 replies; 8+ messages in thread
From: David Edelsohn @ 2013-09-11 21:48 UTC (permalink / raw)
To: GCC Patches, Alan Modra
On Wed, Sep 11, 2013 at 7:12 AM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Sep 09, 2013 at 06:37:03PM +0930, Alan Modra wrote:
>> gcc/
>> * config/rs6000/predicates.md (add_cint_operand): New.
>> (reg_or_add_cint_operand): Use add_cint_operand.
>> * config/rs6000/rs6000.md (largetoc_high_plus): Restrict offset
>> using add_cint_operand.
>> (largetoc_high_plus_aix, small_toc_ref): Likewise.
>> gcc/testsuite/
>> * gcc.target/powerpc/medium_offset.c: New.
>
> I missed seeing one testcase regression caused by this patch. :-(
> gcc.c-torture/compile/pr41634.c at -O3 gets an "insn does not satisfy
> its constraints". Fixed with the following. OK to apply?
>
> * config/rs6000/rs6000.c (toc_relative_expr_p): Use add_cint_operand.
Okay.
Thanks, David
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-09-11 20:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-06 7:13 [RS6000] powerpc64 -mcmodel=medium large symbol offsets Alan Modra
2013-09-06 18:18 ` David Edelsohn
2013-09-06 23:36 ` Alan Modra
2013-09-07 8:03 ` Alan Modra
2013-09-09 9:13 ` Alan Modra
2013-09-09 15:21 ` David Edelsohn
2013-09-11 11:41 ` Alan Modra
2013-09-11 21:48 ` David Edelsohn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).