diff mbox

Fix find_base_term in 32-bit -fpic code (PR lto/64025)

Message ID CAFULd4arq=O1_3pOFXFTzQ16DH=ZZyA8mnT00KDNSo73OteQCQ@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Nov. 25, 2014, 8:13 a.m. UTC
On Tue, Nov 25, 2014 at 8:40 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Nov 25, 2014 at 12:25 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>
>> The fallback delegitimization I've added as last option mainly for
>> debug info purposes, when we don't know if the base is a PIC register
>> or say a PIC register plus some addend, unfortunately in some tests
>> broke find_base_term, which for PLUS looks only at the first operand
>> and recursion on it finds a base term, it returns it immediately.
>> So, it found base term of _GLOBAL_OFFSET_TABLE_, when the right base
>> term is actually in the second operand.
>>
>> This patch fixes it by swapping the operands, debug info doesn't care about
>> the order, it won't match in any instruction anyway, but helps alias.c.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> 2014-11-24  Jakub Jelinek  <jakub@redhat.com>
>>
>>         PR lto/64025
>>         * config/i386/i386.c (ix86_delegitimize_address): Ensure result
>>         comes before (addend - _GLOBAL_OFFSET_TABLE_) term.
>
> Can you also swap operands of (%ecx - %ebx) + foo? There is no point
> digging into RTX involving registers only when we know that we are
> looking for foo. This will also be consistent with the code you
> patched below.

Something like attached prototype patch.

Uros.

Comments

Jakub Jelinek Nov. 25, 2014, 8:20 a.m. UTC | #1
On Tue, Nov 25, 2014 at 09:13:10AM +0100, Uros Bizjak wrote:
> On Tue, Nov 25, 2014 at 8:40 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > On Tue, Nov 25, 2014 at 12:25 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> >
> >> The fallback delegitimization I've added as last option mainly for
> >> debug info purposes, when we don't know if the base is a PIC register
> >> or say a PIC register plus some addend, unfortunately in some tests
> >> broke find_base_term, which for PLUS looks only at the first operand
> >> and recursion on it finds a base term, it returns it immediately.
> >> So, it found base term of _GLOBAL_OFFSET_TABLE_, when the right base
> >> term is actually in the second operand.
> >>
> >> This patch fixes it by swapping the operands, debug info doesn't care about
> >> the order, it won't match in any instruction anyway, but helps alias.c.
> >>
> >> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >>
> >> 2014-11-24  Jakub Jelinek  <jakub@redhat.com>
> >>
> >>         PR lto/64025
> >>         * config/i386/i386.c (ix86_delegitimize_address): Ensure result
> >>         comes before (addend - _GLOBAL_OFFSET_TABLE_) term.
> >
> > Can you also swap operands of (%ecx - %ebx) + foo? There is no point
> > digging into RTX involving registers only when we know that we are
> > looking for foo. This will also be consistent with the code you
> > patched below.
> 
> Something like attached prototype patch.

Actually, thinking about it more, at least according to
commutative_operand_precedence the canonical order is
what we used to return (i.e. (something - _G_O_T_) + (symbol_ref)
or
(something - _G_O_T_) + (const (symbol_ref +- const))
So perhaps better fix is to follow find_base_value, which does something
like:
        /* Guess which operand is the base address:
           If either operand is a symbol, then it is the base.  If
           either operand is a CONST_INT, then the other is the base.  */
        if (CONST_INT_P (src_1) || CONSTANT_P (src_0))
          return find_base_value (src_0);
        else if (CONST_INT_P (src_0) || CONSTANT_P (src_1))
          return find_base_value (src_1);
and do something similar in find_base_term too.  I.e. perhaps even with
higher precedence over REG_P with REG_POINTER (or lower, in these cases
it doesn't really matter, neither argument is REG_P), choose first
operand that is CONSTANT_P and not CONST_INT_P.

	Jakub
diff mbox

Patch

Index: i386.c
===================================================================
--- i386.c	(revision 218037)
+++ i386.c	(working copy)
@@ -14847,19 +14847,20 @@  ix86_delegitimize_address (rtx x)
 	 leal (%ebx, %ecx, 4), %ecx
 	 ...
 	 movl foo@GOTOFF(%ecx), %edx
-	 in which case we return (%ecx - %ebx) + foo
-	 or (%ecx - _GLOBAL_OFFSET_TABLE_) + foo if pseudo_pic_reg
+	 in which case we return foo + (%ecx - %ebx)
+	 or foo + (%ecx - _GLOBAL_OFFSET_TABLE_) if pseudo_pic_reg
 	 and reload has completed.  */
       if (pic_offset_table_rtx
 	  && (!reload_completed || !ix86_use_pseudo_pic_reg ()))
-        result = gen_rtx_PLUS (Pmode, gen_rtx_MINUS (Pmode, copy_rtx (addend),
-						     pic_offset_table_rtx),
-			       result);
+        result = gen_rtx_PLUS (Pmode, result,
+			       gen_rtx_MINUS (Pmode, copy_rtx (addend),
+					      pic_offset_table_rtx));
       else if (pic_offset_table_rtx && !TARGET_MACHO && !TARGET_VXWORKS_RTP)
 	{
 	  rtx tmp = gen_rtx_SYMBOL_REF (Pmode, GOT_SYMBOL_NAME);
-	  tmp = gen_rtx_MINUS (Pmode, copy_rtx (addend), tmp);
-	  result = gen_rtx_PLUS (Pmode, tmp, result);
+	  result = gen_rtx_PLUS (Pmode, result,
+				 gen_rtx_MINUS (Pmode, copy_rtx (addend),
+						tmp));
 	}
       else
 	return orig_x;