Patchwork PATCH [6/n] X32: Supprot 32bit address

login
register
mail settings
Submitter Uros Bizjak
Date July 19, 2011, 9:16 a.m.
Message ID <CAFULd4bFt16CtZk6hrZz3p258Bf1szLG+q2iPFXxZKNcPJJjYA@mail.gmail.com>
Download mbox | patch
Permalink /patch/105423/
State New
Headers show

Comments

Uros Bizjak - July 19, 2011, 9:16 a.m.
On Tue, Jul 19, 2011 at 10:46 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:

>>>>>>>> TARGET_MEM_REF only works on ptr_mode.   This patch allows 32bit address
>>>>>>>> in x32 mode.  OK for trunk?
>>>>>>>
>>>>>>> Do you perhaps have a testcase to help in analyzing the problem?
>>>>>>>
>>>>>>
>>>>>> See:
>>>>>>
>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49780
>>>>>
>>>>> I don't think that tree-ssa-address/addr_for_mem_ref is correct when
>>>>> REALLY_EXPAND is false. It constructs RTX "template" in pointer_mode,
>>>>> which is not necessary valid and is rejected from
>>>>> ix86_validate_address_p. When really expanding the expression, we have
>>>>> a conversion at the end:
>>>>>
>>>>>  gen_addr_rtx (pointer_mode, sym, bse, idx, st, off, &address, NULL, NULL);
>>>>>  if (pointer_mode != address_mode)
>>>>>    address = convert_memory_address (address_mode, address);
>>>>>  return address;
>>>>>
>>>>> This is in fact your r175912 change in the fix for PR47383 - you need
>>>>> to do something with template as well...
>>>>>
>>>>
>>>> Since TARGET_MEM_REF only works on ptr_mode, I don't think
>>>> we can change template.  We just need to accept TARGET_MEM_REF
>>>> in ptr_mode and fix it up later.
>>>
>>> No, a template is used to get some insight into the supported address
>>> structure. If there is a mismatch, this approach fails, we can as well
>>> give the compiler whatever fake template we want.

I have investigated other Pmode != ptr_mode targets, and none of them
check the mode of a register in TARGET_LEGITIMATE_ADDRESS_P (or
equivalent GO_IF_LEGITIMATE_ADDRESS). All it matters is only if there
is a register and regno of the register.

Attached patch simply removes these two checks, as it seems they are
not needed. This also follows how other Pmode != ptr_mode targets.

2011-07-19  Uros Bizjak  <ubizjak@gmail.com>

	PR target/49780
	* config/i386/i386.c (ix86_legitimate_address_p): Remove checks that
	base and index registers are in Pmode.

Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
{,-m32}. Can you please re-test it on x32?

BTW: I still think that template should return the same address
structure as expansion, but this won't crash the compiler anymore.

Uros.
H.J. Lu - July 19, 2011, 1:47 p.m.
On Tue, Jul 19, 2011 at 2:16 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Jul 19, 2011 at 10:46 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>
>>>>>>>>> TARGET_MEM_REF only works on ptr_mode.   This patch allows 32bit address
>>>>>>>>> in x32 mode.  OK for trunk?
>>>>>>>>
>>>>>>>> Do you perhaps have a testcase to help in analyzing the problem?
>>>>>>>>
>>>>>>>
>>>>>>> See:
>>>>>>>
>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49780
>>>>>>
>>>>>> I don't think that tree-ssa-address/addr_for_mem_ref is correct when
>>>>>> REALLY_EXPAND is false. It constructs RTX "template" in pointer_mode,
>>>>>> which is not necessary valid and is rejected from
>>>>>> ix86_validate_address_p. When really expanding the expression, we have
>>>>>> a conversion at the end:
>>>>>>
>>>>>>  gen_addr_rtx (pointer_mode, sym, bse, idx, st, off, &address, NULL, NULL);
>>>>>>  if (pointer_mode != address_mode)
>>>>>>    address = convert_memory_address (address_mode, address);
>>>>>>  return address;
>>>>>>
>>>>>> This is in fact your r175912 change in the fix for PR47383 - you need
>>>>>> to do something with template as well...
>>>>>>
>>>>>
>>>>> Since TARGET_MEM_REF only works on ptr_mode, I don't think
>>>>> we can change template.  We just need to accept TARGET_MEM_REF
>>>>> in ptr_mode and fix it up later.
>>>>
>>>> No, a template is used to get some insight into the supported address
>>>> structure. If there is a mismatch, this approach fails, we can as well
>>>> give the compiler whatever fake template we want.
>
> I have investigated other Pmode != ptr_mode targets, and none of them
> check the mode of a register in TARGET_LEGITIMATE_ADDRESS_P (or
> equivalent GO_IF_LEGITIMATE_ADDRESS). All it matters is only if there
> is a register and regno of the register.
>
> Attached patch simply removes these two checks, as it seems they are
> not needed. This also follows how other Pmode != ptr_mode targets.
>
> 2011-07-19  Uros Bizjak  <ubizjak@gmail.com>
>
>        PR target/49780
>        * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that
>        base and index registers are in Pmode.
>
> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
> {,-m32}. Can you please re-test it on x32?

Comparing with my patch, which only allows DImode and SImode,
it caused the following regressions:

FAIL: libgomp.fortran/omp_atomic1.f90  -O1  execution test
FAIL: libgomp.fortran/omp_atomic1.f90  -O2  execution test
FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer  execution test
FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
-funroll-all-loops -finline-functions  execution test
FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
-funroll-loops  execution test
FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -g  execution test
FAIL: libgomp.fortran/omp_atomic1.f90  -Os  execution test

> BTW: I still think that template should return the same address
> structure as expansion, but this won't crash the compiler anymore.
>
> Uros.
>
Uros Bizjak - July 19, 2011, 2:04 p.m.
On Tue, Jul 19, 2011 at 3:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>> Attached patch simply removes these two checks, as it seems they are
>> not needed. This also follows how other Pmode != ptr_mode targets.
>>
>> 2011-07-19  Uros Bizjak  <ubizjak@gmail.com>
>>
>>        PR target/49780
>>        * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that
>>        base and index registers are in Pmode.
>>
>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
>> {,-m32}. Can you please re-test it on x32?
>
> Comparing with my patch, which only allows DImode and SImode,
> it caused the following regressions:
>
> FAIL: libgomp.fortran/omp_atomic1.f90  -O1  execution test
> FAIL: libgomp.fortran/omp_atomic1.f90  -O2  execution test
> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer  execution test
> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
> -funroll-all-loops -finline-functions  execution test
> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
> -funroll-loops  execution test
> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -g  execution test
> FAIL: libgomp.fortran/omp_atomic1.f90  -Os  execution test
>
>> BTW: I still think that template should return the same address
>> structure as expansion, but this won't crash the compiler anymore.

There is no non-DImode addresses in insn stream, so I doubt the bug is
due to my change.

Uros.
H.J. Lu - July 19, 2011, 2:42 p.m.
On Tue, Jul 19, 2011 at 7:04 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Jul 19, 2011 at 3:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>>> Attached patch simply removes these two checks, as it seems they are
>>> not needed. This also follows how other Pmode != ptr_mode targets.
>>>
>>> 2011-07-19  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>        PR target/49780
>>>        * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that
>>>        base and index registers are in Pmode.
>>>
>>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
>>> {,-m32}. Can you please re-test it on x32?
>>
>> Comparing with my patch, which only allows DImode and SImode,
>> it caused the following regressions:
>>
>> FAIL: libgomp.fortran/omp_atomic1.f90  -O1  execution test
>> FAIL: libgomp.fortran/omp_atomic1.f90  -O2  execution test
>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer  execution test
>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
>> -funroll-all-loops -finline-functions  execution test
>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
>> -funroll-loops  execution test
>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -g  execution test
>> FAIL: libgomp.fortran/omp_atomic1.f90  -Os  execution test
>>
>>> BTW: I still think that template should return the same address
>>> structure as expansion, but this won't crash the compiler anymore.
>
> There is no non-DImode addresses in insn stream, so I doubt the bug is
> due to my change.
>

I saw the same failures on x86-64:

http://gcc.gnu.org/ml/gcc-testresults/2011-07/msg02224.html

Can you take a look?

Thanks.
Uros Bizjak - July 19, 2011, 4:26 p.m.
On Tue, Jul 19, 2011 at 4:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jul 19, 2011 at 7:04 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Tue, Jul 19, 2011 at 3:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>>>> Attached patch simply removes these two checks, as it seems they are
>>>> not needed. This also follows how other Pmode != ptr_mode targets.
>>>>
>>>> 2011-07-19  Uros Bizjak  <ubizjak@gmail.com>
>>>>
>>>>        PR target/49780
>>>>        * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that
>>>>        base and index registers are in Pmode.
>>>>
>>>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
>>>> {,-m32}. Can you please re-test it on x32?
>>>
>>> Comparing with my patch, which only allows DImode and SImode,
>>> it caused the following regressions:
>>>
>>> FAIL: libgomp.fortran/omp_atomic1.f90  -O1  execution test
>>> FAIL: libgomp.fortran/omp_atomic1.f90  -O2  execution test
>>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer  execution test
>>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
>>> -funroll-all-loops -finline-functions  execution test
>>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -fomit-frame-pointer
>>> -funroll-loops  execution test
>>> FAIL: libgomp.fortran/omp_atomic1.f90  -O3 -g  execution test
>>> FAIL: libgomp.fortran/omp_atomic1.f90  -Os  execution test
>>>
>>>> BTW: I still think that template should return the same address
>>>> structure as expansion, but this won't crash the compiler anymore.
>>
>> There is no non-DImode addresses in insn stream, so I doubt the bug is
>> due to my change.
>>
>
> I saw the same failures on x86-64:
>
> http://gcc.gnu.org/ml/gcc-testresults/2011-07/msg02224.html
>
> Can you take a look?

Sometimes, the compiler is really creative in inventing instructions:

(insn 47 46 49 7 (set (reg:SI 68 [ D.1686 ])
        (subreg:SI (plus:SF (reg:SF 159 [ D.1685 ])
                (reg:SF 159 [ D.1685 ])) 0)) omp_atomic1.f90:17 247 {*lea_2}
     (expr_list:REG_DEAD (reg:SF 159 [ D.1685 ])
        (nil)))

Really funny.

Uros.
Jakub Jelinek - July 19, 2011, 4:30 p.m.
On Tue, Jul 19, 2011 at 06:26:33PM +0200, Uros Bizjak wrote:
> Sometimes, the compiler is really creative in inventing instructions:
> 
> (insn 47 46 49 7 (set (reg:SI 68 [ D.1686 ])
>         (subreg:SI (plus:SF (reg:SF 159 [ D.1685 ])
>                 (reg:SF 159 [ D.1685 ])) 0)) omp_atomic1.f90:17 247 {*lea_2}
>      (expr_list:REG_DEAD (reg:SF 159 [ D.1685 ])
>         (nil)))
> 
> Really funny.

That's the job of combiner to try all kinds of stuff and it is the
responsibility of the backend to reject those.  I think it would be better
to get back to testing Pmode in the legitimate address hook, perhaps
allowing ptr_mode too in addition to Pmode (which for -m32/-m64 won't mean
any change, just for -mx32).

	Jakub
Uros Bizjak - July 19, 2011, 4:37 p.m.
On Tue, Jul 19, 2011 at 6:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Jul 19, 2011 at 06:26:33PM +0200, Uros Bizjak wrote:
>> Sometimes, the compiler is really creative in inventing instructions:
>>
>> (insn 47 46 49 7 (set (reg:SI 68 [ D.1686 ])
>>         (subreg:SI (plus:SF (reg:SF 159 [ D.1685 ])
>>                 (reg:SF 159 [ D.1685 ])) 0)) omp_atomic1.f90:17 247 {*lea_2}
>>      (expr_list:REG_DEAD (reg:SF 159 [ D.1685 ])
>>         (nil)))
>>
>> Really funny.
>
> That's the job of combiner to try all kinds of stuff and it is the
> responsibility of the backend to reject those.  I think it would be better
> to get back to testing Pmode in the legitimate address hook, perhaps
> allowing ptr_mode too in addition to Pmode (which for -m32/-m64 won't mean
> any change, just for -mx32).

Actually, there is a bypass in ix86_decompose_address, and this RTX
squeezed through. IMO constructs like this should be rejected in
i_d_a, which effectively only moves Pmode/ptr_mode check here.

I'm looking into it.

Uros.

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 176433)
+++ config/i386/i386.c	(working copy)
@@ -11653,10 +11653,6 @@  ix86_legitimate_address_p (enum machine_
 	/* Base is not a register.  */
 	return false;
 
-      if (GET_MODE (base) != Pmode)
-	/* Base is not in Pmode.  */
-	return false;
-
       if ((strict && ! REG_OK_FOR_BASE_STRICT_P (reg))
 	  || (! strict && ! REG_OK_FOR_BASE_NONSTRICT_P (reg)))
 	/* Base is not valid.  */
@@ -11682,10 +11678,6 @@  ix86_legitimate_address_p (enum machine_
 	/* Index is not a register.  */
 	return false;
 
-      if (GET_MODE (index) != Pmode)
-	/* Index is not in Pmode.  */
-	return false;
-
       if ((strict && ! REG_OK_FOR_INDEX_STRICT_P (reg))
 	  || (! strict && ! REG_OK_FOR_INDEX_NONSTRICT_P (reg)))
 	/* Index is not valid.  */