Re: [PATCH] Fix find_base_term in 32-bit -fpic code (PR lto/64025, take 2)
On Wed, 26 Nov 2014, Jakub Jelinek wrote:
On Tue, Nov 25, 2014 at 09:20:13AM +0100, Jakub Jelinek wrote:
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.
Yeah, that makes sense.
Here it is. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?
Ok.
Thanks,
Richard.
2014-11-26 Jakub Jelinek ja...@redhat.com
PR lto/64025
* alias.c (find_base_term): Use std::swap. Prefer tmp2
if it is CONSTANT_P other than CONST_INT.
--- gcc/alias.c.jj2014-11-21 10:17:17.0 +0100
+++ gcc/alias.c 2014-11-26 12:31:24.719485590 +0100
@@ -1756,11 +1756,11 @@ find_base_term (rtx x)
if (REG_P (tmp1) REG_POINTER (tmp1))
;
else if (REG_P (tmp2) REG_POINTER (tmp2))
- {
- rtx tem = tmp1;
- tmp1 = tmp2;
- tmp2 = tem;
- }
+ std::swap (tmp1, tmp2);
+ /* If second argument is constant which has base term, prefer it
+over variable tmp1. See PR64025. */
+ else if (CONSTANT_P (tmp2) !CONST_INT_P (tmp2))
+ std::swap (tmp1, tmp2);
/* Go ahead and find the base term for both operands. If either base
term is from a pointer or is a named object or a special address
Jakub
--
Richard Biener rguent...@suse.de
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendoerffer, HRB 21284
(AG Nuernberg)
Maxfeldstrasse 5, 90409 Nuernberg, Germany
[PATCH] Fix find_base_term in 32-bit -fpic code (PR lto/64025, take 2)
On Tue, Nov 25, 2014 at 09:20:13AM +0100, Jakub Jelinek wrote:
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.
Here it is. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?
2014-11-26 Jakub Jelinek ja...@redhat.com
PR lto/64025
* alias.c (find_base_term): Use std::swap. Prefer tmp2
if it is CONSTANT_P other than CONST_INT.
--- gcc/alias.c.jj 2014-11-21 10:17:17.0 +0100
+++ gcc/alias.c 2014-11-26 12:31:24.719485590 +0100
@@ -1756,11 +1756,11 @@ find_base_term (rtx x)
if (REG_P (tmp1) REG_POINTER (tmp1))
;
else if (REG_P (tmp2) REG_POINTER (tmp2))
- {
- rtx tem = tmp1;
- tmp1 = tmp2;
- tmp2 = tem;
- }
+ std::swap (tmp1, tmp2);
+ /* If second argument is constant which has base term, prefer it
+ over variable tmp1. See PR64025. */
+ else if (CONSTANT_P (tmp2) !CONST_INT_P (tmp2))
+ std::swap (tmp1, tmp2);
/* Go ahead and find the base term for both operands. If either base
term is from a pointer or is a named object or a special address
Jakub
Re: [PATCH] Fix find_base_term in 32-bit -fpic code (PR lto/64025)
On Tue, Nov 25, 2014 at 8:40 AM, Uros Bizjak ubiz...@gmail.com wrote:
On Tue, Nov 25, 2014 at 12:25 AM, Jakub Jelinek ja...@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 ja...@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.
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;
Re: [PATCH] Fix find_base_term in 32-bit -fpic code (PR lto/64025)
On Tue, Nov 25, 2014 at 09:13:10AM +0100, Uros Bizjak wrote: On Tue, Nov 25, 2014 at 8:40 AM, Uros Bizjak ubiz...@gmail.com wrote: On Tue, Nov 25, 2014 at 12:25 AM, Jakub Jelinek ja...@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 ja...@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
[PATCH] Fix find_base_term in 32-bit -fpic code (PR lto/64025)
Hi!
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 ja...@redhat.com
PR lto/64025
* config/i386/i386.c (ix86_delegitimize_address): Ensure result
comes before (addend - _GLOBAL_OFFSET_TABLE_) term.
--- gcc/config/i386/i386.c.jj 2014-11-24 10:26:26.0 +0100
+++ gcc/config/i386/i386.c 2014-11-24 20:17:27.659091709 +0100
@@ -14848,7 +14848,7 @@ ix86_delegitimize_address (rtx x)
...
movl foo@GOTOFF(%ecx), %edx
in which case we return (%ecx - %ebx) + foo
-or (%ecx - _GLOBAL_OFFSET_TABLE_) + foo if pseudo_pic_reg
+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 ()))
@@ -14859,7 +14859,14 @@ ix86_delegitimize_address (rtx x)
{
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);
+ /* The order of the operands here is very important. find_base_term
+will only recurse into the first operand of PLUS if none of the
+arguments is REG with REG_POINTER_P set on it, and if that finds
+base term, it doesn't recurse into the second operand of PLUS.
+We don't want find_base_term to return the artificial
+_GLOBAL_OFFSET_TABLE_ symbol, so ensure it goes into the
+second operand. */
+ result = gen_rtx_PLUS (Pmode, result, tmp);
}
else
return orig_x;
Jakub
Re: [PATCH] Fix find_base_term in 32-bit -fpic code (PR lto/64025)
On Tue, Nov 25, 2014 at 12:25 AM, Jakub Jelinek ja...@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 ja...@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.
Uros.
--- gcc/config/i386/i386.c.jj 2014-11-24 10:26:26.0 +0100
+++ gcc/config/i386/i386.c 2014-11-24 20:17:27.659091709 +0100
@@ -14848,7 +14848,7 @@ ix86_delegitimize_address (rtx x)
...
movl foo@GOTOFF(%ecx), %edx
in which case we return (%ecx - %ebx) + foo
-or (%ecx - _GLOBAL_OFFSET_TABLE_) + foo if pseudo_pic_reg
+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 ()))
@@ -14859,7 +14859,14 @@ ix86_delegitimize_address (rtx x)
{
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);
+ /* The order of the operands here is very important. find_base_term
+will only recurse into the first operand of PLUS if none of the
+arguments is REG with REG_POINTER_P set on it, and if that finds
+base term, it doesn't recurse into the second operand of PLUS.
+We don't want find_base_term to return the artificial
+_GLOBAL_OFFSET_TABLE_ symbol, so ensure it goes into the
+second operand. */
+ result = gen_rtx_PLUS (Pmode, result, tmp);
}
else
return orig_x;
Jakub
