Patchwork [RS6000] powerpc64 -mcmodel=medium large symbol offsets

login
register
mail settings
Submitter Alan Modra
Date Sept. 6, 2013, 7:13 a.m.
Message ID <20130906071329.GB3430@bubble.grove.modra.org>
Download mbox | patch
Permalink /patch/273098/
State New
Headers show

Comments

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.
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

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