[RS6000] powerpc64 -mcmodel=medium large symbol offsets

Submitted by Alan Modra on Sept. 6, 2013, 7:13 a.m.

Details

Message ID 20130906071329.GB3430@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra Sept. 6, 2013, 7:13 a.m.
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.

Comments

David Edelsohn Sept. 6, 2013, 6:18 p.m.
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
Alan Modra Sept. 6, 2013, 11:36 p.m.
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 Sept. 7, 2013, 2:44 a.m.
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.

Patch hide | download patch | download mbox

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)")