| Message ID | 20130906071329.GB3430@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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)")
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.