Re: Patch: PR target/55142: [4.8 Regression] internal compiler error: in plus_constant, at explow.c:88

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Nov 13, 2012 at 7:23 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>>> >>>> Since x32 runs in 64-bit mode, for address -0x40000300(%rax), hardware
>>>> >>>> sign-extends displacement from 32-bits to 64-bits and adds it to %rax.
>>>> >>>> But x32 wants 32-bit -0x40000300, not 64-bit -0x40000300.  This patch
>>>> >>>> uses 32-bit registers instead of 64-bit registers when displacement
>>>> >>>> < -16*1024*1024.  -16*1024*1024 is used instead of 0 so that we will
>>>> >>>> still generate -16(%rsp) instead of -16(%esp).
>>>> >>>>
>>>> >>>> Tested it on Linux/x32.  OK to install?
>>>> >>>
>>>> >>> This problem uncovers a bug in the middle-end, so I guess it would be
>>>> >>> better to fix it there.
>>>> >>>
>>>> >>> Uros.
>>>> >>
>>>> >> The problem is it isn't safe to transform
>>>> >>
>>>> >> (zero_extend:DI (plus:SI (FOO:SI) (const_int Y)))
>>>> >>
>>>> >> to
>>>> >>
>>>> >> (plus:DI (zero_extend:DI (FOO:SI)) (const_int Y))
>>>> >>
>>>> >> when Y is negative and its absolute value is greater than FOO.  However,
>>>> >> we have to do this transformation since other parts of GCC depend on
>>>> >> it.  If we revert the fix for
>>>> >>
>>>> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49721
>>>> >>
>>>> >> we will get
>>>> >>
>>>> >> FAIL: gcc.c-torture/compile/990523-1.c  -O3 -g  (internal compiler error)
>>>> >> FAIL: gcc.c-torture/compile/990523-1.c  -O3 -g  (test for excess errors)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -fomit-frame-pointer -funroll-all-loo
>>>> >> ps -finline-functions  (internal compiler error)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -fomit-frame-pointer -funroll-all-loo
>>>> >> ps -finline-functions  (test for excess errors)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -fomit-frame-pointer -funroll-loops
>>>> >> (internal compiler error)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -fomit-frame-pointer -funroll-loops
>>>> >> (test for excess errors)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -fomit-frame-pointer  (internal compi
>>>> >> ler error)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -fomit-frame-pointer  (test for exces
>>>> >> s errors)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -g  (internal compiler error)
>>>> >> FAIL: gcc.c-torture/compile/pr41634.c  -O3 -g  (test for excess errors)
>>>> >> FAIL: gcc.dg/Warray-bounds.c (internal compiler error)
>>>> >> FAIL: gcc.dg/Warray-bounds.c (test for excess errors)
>>>> >>
>>>> >> since we generate pseudo registers to convert SImode to DImode
>>>> >> after reload.  Fixing it requires significant changes.
>>>> >>
>>>> >> This is only a problem for 64-bit register address since the symbolic
>>>> >> address is 32-bit.  Using 32-bit base/index registers will work around
>>>> >> this issue.
>>>> >
>>>> > This address
>>>> >
>>>> > (plus:DI (zero_extend:DI (FOO:SI)) (const_int Y))
>>>> >
>>>> > is OK for x32 as long as Y, which is encoded as 32-bit immediate,
>>>> > is zero-extend from 32-bit to 64-bit.  SImode address does it.
>>>> > My patch optimizes it a little bit by using SImode address only
>>>> > for Y < -16*1024*1024.
>>>>
>>>> I was wondering, why we operate with constant -16*1024*1024? Should we
>>>> use 0x7FFFFFF instead? Since the MSB is always zero, we are safe.
>>>>
>>>
>>> We can check 0x7FFFFFF, i.e., disp < 0.  I use -16*1024*1024, which
>>> is also used to check legitimate address displacements for PIC, to
>>> reduce code sizes for small negative displacements.  Or we can always
>>> encode negative displacements with zero-extension, including -1(%rsp).
>>>
>>>> Please add a fat ??? comment, why we paper-over this issue and repost
>>>> the latest patch. I got lost in all the versions :(
>>>>
>>>
>>> Here is the updated patch.
>>>
>>> gcc/
>>>
>>> 2012-11-13  Eric Botcazou  <ebotcazou@adacore.com>
>>>             H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>         PR target/55142
>>>         * config/i386/i386.c (legitimize_pic_address): Properly handle
>>>         REG + CONST.
>>>         (ix86_print_operand_address): For x32, zero-extend negative
>>>         displacement if it < -16*1024*1024.
>>>
>>> gcc/testsuite/
>>>
>>> 2012-11-13  H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>         PR target/55142
>>>         * gcc.target/i386/pr55142-1.c: New file.
>>>         * gcc.target/i386/pr55142-2.c: Likewise.
>>
>> OK, for mainline SVN (with the reservation that middle-end fix was not
>> found to be a viable solution, so target fix is papering-over real
>> issue. Let's wait for the next victim... ).
>
> That is true.
>
>> Oh, and please fix a typo of mine in the one line above the patch
>> hunk; the modifier for SI addresses should be 'k', not 'l'.
>
> Will do.
>
>> BTW: Do we need this patch also for 4.7? x32 address-mode is long by
>> default there, but the problem doesn't trigger.
>>
>
> This regression is triggered by revision 188008:
>
> http://gcc.gnu.org/ml/gcc-cvs/2012-05/msg00038.html
>
> aka the un-sign-extension of sizetype constants.  We can
> backport it to 4.7 branch if we want to be on the safer side.

Yes, please backport the patch to 4.7 after it lives a couple of days
in mainline without problems.

Thanks,
Uros.