Patchwork PATCH: PR target/49853: [x32] PIC doesn't work with external symbol

login
register
mail settings
Submitter Uros Bizjak
Date July 26, 2011, 5:26 p.m.
Message ID <CAFULd4bT=73kBz3WPoCwvVi=ULswgsbGcinY3Y4Uf7thEqpPmA@mail.gmail.com>
Download mbox | patch
Permalink /patch/106898/
State New
Headers show

Comments

Uros Bizjak - July 26, 2011, 5:26 p.m.
On Tue, Jul 26, 2011 at 4:59 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:

> This patch fixes PIC with external symbol and updates
> x86_64_immediate_operand/x86_64_zext_immediate_operand/x86_64_movabs_operand
> for x32.

> 2011-07-26  H.J. Lu  <hongjiu.lu@intel.com>
>
>        PR target/49853
>        * config/i386/i386.c (ix86_expand_move): Call convert_to_mode
>        on legitimize_tls_address return if needed.  Allow ptr_mode for
>        symbolic operand with PIC.

Eh... half of your patch is just an unnecessary rename of a temporary
variable. See attached patch for a cleaned-up version.
Also, please use explicit DImode and SImode checks to match what
ix86_legitimate_address_p does.

>        * config/i386/predicates.md (x86_64_immediate_operand): Always
>        allow the offsetted memory references for TARGET_X32.
>        (x86_64_zext_immediate_operand): Likewise.
>        (x86_64_movabs_operand): Don't allow nonmemory_operand for
>        TARGET_X32.

Why? It is certainly not needed for -fPIC. Please provide a separate
patch and testcase for predicates.md change.

Uros.
H.J. Lu - July 26, 2011, 5:31 p.m.
On Tue, Jul 26, 2011 at 10:26 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Jul 26, 2011 at 4:59 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>
>> This patch fixes PIC with external symbol and updates
>> x86_64_immediate_operand/x86_64_zext_immediate_operand/x86_64_movabs_operand
>> for x32.
>
>> 2011-07-26  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>        PR target/49853
>>        * config/i386/i386.c (ix86_expand_move): Call convert_to_mode
>>        on legitimize_tls_address return if needed.  Allow ptr_mode for
>>        symbolic operand with PIC.
>
> Eh... half of your patch is just an unnecessary rename of a temporary
> variable. See attached patch for a cleaned-up version.

It looks good to me.  Can you check it in?

> Also, please use explicit DImode and SImode checks to match what
> ix86_legitimate_address_p does.
>
>>        * config/i386/predicates.md (x86_64_immediate_operand): Always
>>        allow the offsetted memory references for TARGET_X32.
>>        (x86_64_zext_immediate_operand): Likewise.
>>        (x86_64_movabs_operand): Don't allow nonmemory_operand for
>>        TARGET_X32.
>
> Why? It is certainly not needed for -fPIC. Please provide a separate
> patch and testcase for predicates.md change.
>

I will submit them separately with some testcases after your patch
is checked in.

Thanks.
Uros Bizjak - July 26, 2011, 5:36 p.m.
On Tue, Jul 26, 2011 at 7:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>> This patch fixes PIC with external symbol and updates
>>> x86_64_immediate_operand/x86_64_zext_immediate_operand/x86_64_movabs_operand
>>> for x32.
>>
>>> 2011-07-26  H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>        PR target/49853
>>>        * config/i386/i386.c (ix86_expand_move): Call convert_to_mode
>>>        on legitimize_tls_address return if needed.  Allow ptr_mode for
>>>        symbolic operand with PIC.
>>
>> Eh... half of your patch is just an unnecessary rename of a temporary
>> variable. See attached patch for a cleaned-up version.
>
> It looks good to me.  Can you check it in?

Please, can you test it on x32 first? I will commit it after
bootstrap/regtest finish.

Thanks,
Uros.
H.J. Lu - July 26, 2011, 5:50 p.m.
On Tue, Jul 26, 2011 at 10:36 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Jul 26, 2011 at 7:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>>>> This patch fixes PIC with external symbol and updates
>>>> x86_64_immediate_operand/x86_64_zext_immediate_operand/x86_64_movabs_operand
>>>> for x32.
>>>
>>>> 2011-07-26  H.J. Lu  <hongjiu.lu@intel.com>
>>>>
>>>>        PR target/49853
>>>>        * config/i386/i386.c (ix86_expand_move): Call convert_to_mode
>>>>        on legitimize_tls_address return if needed.  Allow ptr_mode for
>>>>        symbolic operand with PIC.
>>>
>>> Eh... half of your patch is just an unnecessary rename of a temporary
>>> variable. See attached patch for a cleaned-up version.
>>
>> It looks good to me.  Can you check it in?
>
> Please, can you test it on x32 first? I will commit it after
> bootstrap/regtest finish.
>

It may need other changes for TLS support.  I can update it
after your change is checked in.
Uros Bizjak - July 26, 2011, 6:05 p.m.
On Tue, Jul 26, 2011 at 7:50 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>>>> This patch fixes PIC with external symbol and updates
>>>>> x86_64_immediate_operand/x86_64_zext_immediate_operand/x86_64_movabs_operand
>>>>> for x32.
>>>>
>>>>> 2011-07-26  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>
>>>>>        PR target/49853
>>>>>        * config/i386/i386.c (ix86_expand_move): Call convert_to_mode
>>>>>        on legitimize_tls_address return if needed.  Allow ptr_mode for
>>>>>        symbolic operand with PIC.
>>>>
>>>> Eh... half of your patch is just an unnecessary rename of a temporary
>>>> variable. See attached patch for a cleaned-up version.
>>>
>>> It looks good to me.  Can you check it in?
>>
>> Please, can you test it on x32 first? I will commit it after
>> bootstrap/regtest finish.
>>
>
> It may need other changes for TLS support.  I can update it
> after your change is checked in.

Committed with following ChangeLog:

2011-07-26  Uros Bizjak  <ubizjak@gmail.com>
	    H.J. Lu  <hongjiu.lu@intel.com>

	PR target/47369
	PR target/49853
	* config/i386/i386.c (ix86_expand_move): Call convert_to_mode
	if legitimize_tls_address returned operand in wrong mode. Allow
	SImode and DImode symbolic operand for PIC.  Call convert_to_mode
	if legitimize_pic_address returned operand in wrong mode.

Tested on x86_64-pc-linux-gnu {,-m32}.

Uros.

Patch

Index: i386.c
===================================================================
--- i386.c	(revision 176794)
+++ i386.c	(working copy)
@@ -15028,11 +15028,14 @@  ix86_expand_move (enum machine_mode mode
 				     op0, 1, OPTAB_DIRECT);
 	  if (tmp == op0)
 	    return;
+	  if (GET_MODE (tmp) != mode)
+	    op1 = convert_to_mode (mode, tmp, 1);
 	}
     }
 
   if ((flag_pic || MACHOPIC_INDIRECT) 
-       && mode == Pmode && symbolic_operand (op1, Pmode))
+      && (mode == SImode || mode == DImode)
+      && symbolic_operand (op1, mode))
     {
       if (TARGET_MACHO && !TARGET_64BIT)
 	{
@@ -15073,13 +15076,15 @@  ix86_expand_move (enum machine_mode mode
       else
 	{
 	  if (MEM_P (op0))
-	    op1 = force_reg (Pmode, op1);
-	  else if (!TARGET_64BIT || !x86_64_movabs_operand (op1, Pmode))
+	    op1 = force_reg (mode, op1);
+	  else if (!TARGET_64BIT || !x86_64_movabs_operand (op1, mode))
 	    {
 	      rtx reg = can_create_pseudo_p () ? NULL_RTX : op0;
 	      op1 = legitimize_pic_address (op1, reg);
 	      if (op0 == op1)
 		return;
+	      if (GET_MODE (op1) != mode)
+		op1 = convert_to_mode (mode, op1, 1);
 	    }
 	}
     }