Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

Hi Jeff,


this patch tries to fix the handling of pic_offset_rtx + 
const(unspec(symbol_ref)+ const_int) in may_trap_p,
and restores the original state of affairs in update_equiv_regs.


What do you think is it OK to extract the symbol_ref out
of the unspec in this way, or is does it need a target hook?


Patch works at least for x86_64 and arm.
Is it OK for trunk?


Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr79286.diff --]
[-- Type: text/x-patch; name="patch-pr79286.diff", Size: 2459 bytes --]

2017-04-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        rtl-optimizatoin/79286
        * ira.c (update_equiv_regs): Revert to using may_tap_p again.
        * rtlanal.c (rtx_addr_can_trap_p_1): SYMBOL_REF_FUNCTION_P can never
	trap.  Extract constant offset from pic_offset_table_rtx +
	const(unspec(symbol_ref)+int_val) and pic_offset_table_rtx +
	const(unspec(symbol_ref)), otherwise RTL may trap.

Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 247077)
+++ gcc/ira.c	(working copy)
@@ -3551,7 +3551,8 @@ update_equiv_regs (void)
 	  if (DF_REG_DEF_COUNT (regno) == 1
 	      && note
 	      && !rtx_varies_p (XEXP (note, 0), 0)
-	      && def_dominates_uses (regno))
+	      && (!may_trap_or_fault_p (XEXP (note, 0))
+		  || def_dominates_uses (regno)))
 	    {
 	      rtx note_value = XEXP (note, 0);
 	      remove_note (insn, note);
Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 247077)
+++ gcc/rtlanal.c	(working copy)
@@ -485,7 +485,7 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST_WIDE_INT
     case SYMBOL_REF:
       if (SYMBOL_REF_WEAK (x))
 	return 1;
-      if (!CONSTANT_POOL_ADDRESS_P (x))
+      if (!CONSTANT_POOL_ADDRESS_P (x) && !SYMBOL_REF_FUNCTION_P (x))
 	{
 	  tree decl;
 	  HOST_WIDE_INT decl_size;
@@ -645,8 +645,23 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST_WIDE_INT
     case PLUS:
       /* An address is assumed not to trap if:
          - it is the pic register plus a constant.  */
-      if (XEXP (x, 0) == pic_offset_table_rtx && CONSTANT_P (XEXP (x, 1)))
-	return 0;
+      if (XEXP (x, 0) == pic_offset_table_rtx
+	  && GET_CODE (XEXP (x, 1)) == CONST)
+	{
+	  x = XEXP (XEXP (x, 1), 0);
+	  if (GET_CODE (x) == UNSPEC
+	      && GET_CODE (XVECEXP (x, 0, 0)) == SYMBOL_REF)
+	    return rtx_addr_can_trap_p_1(XVECEXP (x, 0, 0),
+					 offset, size, mode, unaligned_mems);
+	  if (GET_CODE (x) == PLUS
+	      && GET_CODE (XEXP (x, 0)) == UNSPEC
+	      && GET_CODE (XVECEXP (XEXP (x, 0), 0, 0)) == SYMBOL_REF
+	      && CONST_INT_P (XEXP (x, 1)))
+	    return rtx_addr_can_trap_p_1(XVECEXP (XEXP (x, 0), 0, 0),
+					 offset + INTVAL (XEXP (x, 1)),
+					 size, mode, unaligned_mems);
+	  return 1;
+	}
 
       /* - or it is an address that can't trap plus a constant integer.  */
       if (CONST_INT_P (XEXP (x, 1))

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Jeff Law]

On 04/23/2017 05:54 AM, Bernd Edlinger wrote:
> Hi Jeff,
> 
> 
> this patch tries to fix the handling of pic_offset_rtx +
> const(unspec(symbol_ref)+ const_int) in may_trap_p,
> and restores the original state of affairs in update_equiv_regs.
> 
> 
> What do you think is it OK to extract the symbol_ref out
> of the unspec in this way, or is does it need a target hook?
> 
> 
> Patch works at least for x86_64 and arm.
> Is it OK for trunk?
> 
> 
> Bernd.
> 
> 
> patch-pr79286.diff
> 
> 
> 2017-04-23  Bernd Edlinger<bernd.edlinger@hotmail.de>
> 
>          rtl-optimizatoin/79286
>          * ira.c (update_equiv_regs): Revert to using may_tap_p again.
>          * rtlanal.c (rtx_addr_can_trap_p_1): SYMBOL_REF_FUNCTION_P can never
> 	trap.  Extract constant offset from pic_offset_table_rtx +
> 	const(unspec(symbol_ref)+int_val) and pic_offset_table_rtx +
> 	const(unspec(symbol_ref)), otherwise RTL may trap.
ISTM that rtx_addr_can_trap_p_1 is fundamentally broken in that 
references via pic_offset_table_rtx can certainly trap.

Whether or not a given reference traps is a function of the size of 
table (not known until link time) and the CONST_INT within the address 
expression.  So I'd be more inclined to remove the special casing of PIC 
addresses where entirely -- that seemed pretty risky during stage3, but 
now would be an appropriate time to tackle that.

If we were to try and keep the special handling, you certainly can't 
depend on the form looking like (const(unspec(symbol_ref) + const_int).

You could have high/lo_sums were and probably other forms too. We allow 
the backends to largely define what  PIC address looks like.

Jeff

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

On 04/28/17 18:48, Jeff Law wrote:
> On 04/23/2017 05:54 AM, Bernd Edlinger wrote:
>> Hi Jeff,
>>
>>
>> this patch tries to fix the handling of pic_offset_rtx +
>> const(unspec(symbol_ref)+ const_int) in may_trap_p,
>> and restores the original state of affairs in update_equiv_regs.
>>
>>
>> What do you think is it OK to extract the symbol_ref out
>> of the unspec in this way, or is does it need a target hook?
>>
>>
>> Patch works at least for x86_64 and arm.
>> Is it OK for trunk?
>>
>>
>> Bernd.
>>
>>
>> patch-pr79286.diff
>>
>>
>> 2017-04-23  Bernd Edlinger<bernd.edlinger@hotmail.de>
>>
>>          rtl-optimizatoin/79286
>>          * ira.c (update_equiv_regs): Revert to using may_tap_p again.
>>          * rtlanal.c (rtx_addr_can_trap_p_1): SYMBOL_REF_FUNCTION_P
>> can never
>>     trap.  Extract constant offset from pic_offset_table_rtx +
>>     const(unspec(symbol_ref)+int_val) and pic_offset_table_rtx +
>>     const(unspec(symbol_ref)), otherwise RTL may trap.
> ISTM that rtx_addr_can_trap_p_1 is fundamentally broken in that
> references via pic_offset_table_rtx can certainly trap.
>
> Whether or not a given reference traps is a function of the size of
> table (not known until link time) and the CONST_INT within the address
> expression.  So I'd be more inclined to remove the special casing of PIC
> addresses where entirely -- that seemed pretty risky during stage3, but
> now would be an appropriate time to tackle that.
>
> If we were to try and keep the special handling, you certainly can't
> depend on the form looking like (const(unspec(symbol_ref) + const_int).
>
> You could have high/lo_sums were and probably other forms too. We allow
> the backends to largely define what  PIC address looks like.
>

Yes I agree, that is probably not worth it.  So I could try to remove
the special handling of PIC+const and see what happens.

However the SYMBOL_REF_FUNCTION_P is another story, that part I would
like to keep: It happens quite often, already w/o -fpic that call 
statements are using SYMBOL_REFs to ordinary (not weak) function
symbols, and may_trap returns 1 for these call statements wihch is IMHO
wrong.


Bernd.

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Jeff Law]

On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>
> 
> Yes I agree, that is probably not worth it.  So I could try to remove
> the special handling of PIC+const and see what happens.
> 
> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
> like to keep: It happens quite often, already w/o -fpic that call
> statements are using SYMBOL_REFs to ordinary (not weak) function
> symbols, and may_trap returns 1 for these call statements wihch is IMHO
> wrong.
Hmm, thinking more about this, wasn't the original case a PIC referrence 
for something like &x[BIGNUM].

Perhaps we could consider a PIC reference without other arithmetic as 
safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you want 
as well good deal many more PIC references as non-trapping.

Jeff

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

On 04/28/17 20:46, Jeff Law wrote:
> On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>>
>>
>> Yes I agree, that is probably not worth it.  So I could try to remove
>> the special handling of PIC+const and see what happens.
>>
>> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
>> like to keep: It happens quite often, already w/o -fpic that call
>> statements are using SYMBOL_REFs to ordinary (not weak) function
>> symbols, and may_trap returns 1 for these call statements wihch is IMHO
>> wrong.
> Hmm, thinking more about this, wasn't the original case a PIC referrence
> for something like &x[BIGNUM].
>
> Perhaps we could consider a PIC reference without other arithmetic as
> safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you want
> as well good deal many more PIC references as non-trapping.
>
> Jeff

Yes, IIRC it was a UNSPEC_GOTOFF.
I think it comes from legitimize_pic_address:

       if (GET_CODE (addr) == PLUS)
           {
             new_rtx = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, XEXP (addr, 0)),
                                       UNSPEC_GOTOFF);
             new_rtx = gen_rtx_PLUS (Pmode, new_rtx, XEXP (addr, 1));
           }
         else
           new_rtx = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, addr), 
UNSPEC_GOTOFF);

       new_rtx = gen_rtx_CONST (Pmode, new_rtx);

and it is somehow special, because it is a static value
that is accessed relative to the PIC register,
we know the bounds of the static object, the form of the
RTL may vary dependent on the target, of course, if the
form is not recognized, may_trap_p would behave as if
the PIC+const case was not there.  Maybe I could check
that the SYMBOL_REF is a local value?

Everything else is accessing the address of an external
variable, this is arranged by the linker and should be safe.


Bernd.

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

On 04/28/17 21:14, Bernd Edlinger wrote:
> On 04/28/17 20:46, Jeff Law wrote:
>> On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>>>
>>>
>>> Yes I agree, that is probably not worth it.  So I could try to remove
>>> the special handling of PIC+const and see what happens.
>>>
>>> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
>>> like to keep: It happens quite often, already w/o -fpic that call
>>> statements are using SYMBOL_REFs to ordinary (not weak) function
>>> symbols, and may_trap returns 1 for these call statements wihch is IMHO
>>> wrong.
>> Hmm, thinking more about this, wasn't the original case a PIC referrence
>> for something like &x[BIGNUM].
>>
>> Perhaps we could consider a PIC reference without other arithmetic as
>> safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you want
>> as well good deal many more PIC references as non-trapping.
>>
>> Jeff
>
> Yes, IIRC it was a UNSPEC_GOTOFF.
> I think it comes from legitimize_pic_address:
>
>       if (GET_CODE (addr) == PLUS)
>           {
>             new_rtx = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, XEXP (addr, 0)),
>                                       UNSPEC_GOTOFF);
>             new_rtx = gen_rtx_PLUS (Pmode, new_rtx, XEXP (addr, 1));
>           }
>         else
>           new_rtx = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, addr),
> UNSPEC_GOTOFF);
>
>       new_rtx = gen_rtx_CONST (Pmode, new_rtx);
>
> and it is somehow special, because it is a static value
> that is accessed relative to the PIC register,
> we know the bounds of the static object, the form of the
> RTL may vary dependent on the target, of course, if the
> form is not recognized, may_trap_p would behave as if
> the PIC+const case was not there.  Maybe I could check
> that the SYMBOL_REF is a local value?
>
> Everything else is accessing the address of an external
> variable, this is arranged by the linker and should be safe.
>
>

Reading a bit further in legitimize_pic_address I see this:

           new_rtx = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, addr), 
UNSPEC_GOT);
           new_rtx = gen_rtx_CONST (Pmode, new_rtx);
           if (TARGET_64BIT)
             new_rtx = force_reg (Pmode, new_rtx);
           new_rtx = gen_rtx_PLUS (Pmode, pic_offset_table_rtx, new_rtx);
           new_rtx = gen_const_mem (Pmode, new_rtx);
           set_mem_alias_set (new_rtx, ix86_GOT_alias_set ());

and gen_const_mem sets MEM_NOTRAP_P
furthermore in may_trap_p_1 we have:

      case MEM:
       /* Recognize specific pattern of stack checking probes.  */
       if (flag_stack_check
           && MEM_VOLATILE_P (x)
           && XEXP (x, 0) == stack_pointer_rtx)
         return 1;
       if (/* MEM_NOTRAP_P only relates to the actual position of the memory
              reference; moving it out of context such as when moving code
              when optimizing, might cause its address to become 
invalid.  */
           code_changed
           || !MEM_NOTRAP_P (x))
         {
           HOST_WIDE_INT size = MEM_SIZE_KNOWN_P (x) ? MEM_SIZE (x) : 0;
           return rtx_addr_can_trap_p_1 (XEXP (x, 0), 0, size,
                                         GET_MODE (x), code_changed);
         }

       return 0;


So it is quite possible that the real pic refernces will not
go into rtx_addr_can_trap_p_1 at all.


Bernd.

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

On 04/28/17 20:46, Jeff Law wrote:
> On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>>
>>
>> Yes I agree, that is probably not worth it.  So I could try to remove
>> the special handling of PIC+const and see what happens.
>>
>> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
>> like to keep: It happens quite often, already w/o -fpic that call
>> statements are using SYMBOL_REFs to ordinary (not weak) function
>> symbols, and may_trap returns 1 for these call statements wihch is IMHO
>> wrong.
> Hmm, thinking more about this, wasn't the original case a PIC referrence
> for something like &x[BIGNUM].
>
> Perhaps we could consider a PIC reference without other arithmetic as
> safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you want
> as well good deal many more PIC references as non-trapping.
>

Yes, I like this idea.

I tried to compile openssl with -m32 -fpic as an example, and counted
how often the mem[pic+const] is hit: that was 2353 times, all kind of
object refs.

Then I tried your idea, and only 54 unhandled pic refs remained, all of
them looking like this:

(plus:SI (reg:SI 107)
     (const:SI (plus:SI (unspec:SI [
                     (symbol_ref:SI ("bf_init") [flags 0x2] <var_decl 
0x2ac00f7bac60 bf_init>)
                 ] UNSPEC_GOTOFF)
             (const_int 4164 [0x1044]))))

I believe that is a negligible fall out from such a big code base.

Although the pic references do no longer reach the
SYMBOL_REF_FUNCTION_P in this version of the patch, I still see
that happening without -fpic option, so I left it as is.


Attached is the new version of my patch.

Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr79286.diff --]
[-- Type: text/x-patch; name="patch-pr79286.diff", Size: 1895 bytes --]

2017-04-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        rtl-optimizatoin/79286
        * ira.c (update_equiv_regs): Revert to using may_tap_p again.
        * rtlanal.c (rtx_addr_can_trap_p_1): SYMBOL_REF_FUNCTION_P can never
	trap.  PIC register plus a const unspec without offset can never trap.

Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 247397)
+++ gcc/ira.c	(working copy)
@@ -3551,7 +3551,8 @@ update_equiv_regs (void)
 	  if (DF_REG_DEF_COUNT (regno) == 1
 	      && note
 	      && !rtx_varies_p (XEXP (note, 0), 0)
-	      && def_dominates_uses (regno))
+	      && (!may_trap_or_fault_p (XEXP (note, 0))
+		  || def_dominates_uses (regno)))
 	    {
 	      rtx note_value = XEXP (note, 0);
 	      remove_note (insn, note);
Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 247397)
+++ gcc/rtlanal.c	(working copy)
@@ -485,7 +485,7 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST_WIDE_INT
     case SYMBOL_REF:
       if (SYMBOL_REF_WEAK (x))
 	return 1;
-      if (!CONSTANT_POOL_ADDRESS_P (x))
+      if (!CONSTANT_POOL_ADDRESS_P (x) && !SYMBOL_REF_FUNCTION_P (x))
 	{
 	  tree decl;
 	  HOST_WIDE_INT decl_size;
@@ -644,8 +644,11 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST_WIDE_INT
 
     case PLUS:
       /* An address is assumed not to trap if:
-         - it is the pic register plus a constant.  */
-      if (XEXP (x, 0) == pic_offset_table_rtx && CONSTANT_P (XEXP (x, 1)))
+	 - it is the pic register plus a const unspec without offset.  */
+      if (XEXP (x, 0) == pic_offset_table_rtx
+	  && GET_CODE (XEXP (x, 1)) == CONST
+	  && GET_CODE (XEXP (XEXP (x, 1), 0)) == UNSPEC
+	  && offset == 0)
 	return 0;
 
       /* - or it is an address that can't trap plus a constant integer.  */

[PING][PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

Ping...

On 04/29/17 09:06, Bernd Edlinger wrote:
> On 04/28/17 20:46, Jeff Law wrote:
>> On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>>>
>>>
>>> Yes I agree, that is probably not worth it.  So I could try to remove
>>> the special handling of PIC+const and see what happens.
>>>
>>> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
>>> like to keep: It happens quite often, already w/o -fpic that call
>>> statements are using SYMBOL_REFs to ordinary (not weak) function
>>> symbols, and may_trap returns 1 for these call statements wihch is IMHO
>>> wrong.
>> Hmm, thinking more about this, wasn't the original case a PIC referrence
>> for something like &x[BIGNUM].
>>
>> Perhaps we could consider a PIC reference without other arithmetic as
>> safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you want
>> as well good deal many more PIC references as non-trapping.
>>
>
> Yes, I like this idea.
>
> I tried to compile openssl with -m32 -fpic as an example, and counted
> how often the mem[pic+const] is hit: that was 2353 times, all kind of
> object refs.
>
> Then I tried your idea, and only 54 unhandled pic refs remained, all of
> them looking like this:
>
> (plus:SI (reg:SI 107)
>     (const:SI (plus:SI (unspec:SI [
>                     (symbol_ref:SI ("bf_init") [flags 0x2] <var_decl
> 0x2ac00f7bac60 bf_init>)
>                 ] UNSPEC_GOTOFF)
>             (const_int 4164 [0x1044]))))
>
> I believe that is a negligible fall out from such a big code base.
>
> Although the pic references do no longer reach the
> SYMBOL_REF_FUNCTION_P in this version of the patch, I still see
> that happening without -fpic option, so I left it as is.
>
>
> Attached is the new version of my patch.
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.

[PING**2][PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

Ping...

On 05/12/17 18:48, Bernd Edlinger wrote:
> Ping...
> 
> On 04/29/17 09:06, Bernd Edlinger wrote:
>> On 04/28/17 20:46, Jeff Law wrote:
>>> On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>>>>
>>>>
>>>> Yes I agree, that is probably not worth it.  So I could try to remove
>>>> the special handling of PIC+const and see what happens.
>>>>
>>>> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
>>>> like to keep: It happens quite often, already w/o -fpic that call
>>>> statements are using SYMBOL_REFs to ordinary (not weak) function
>>>> symbols, and may_trap returns 1 for these call statements wihch is IMHO
>>>> wrong.
>>> Hmm, thinking more about this, wasn't the original case a PIC referrence
>>> for something like &x[BIGNUM].
>>>
>>> Perhaps we could consider a PIC reference without other arithmetic as
>>> safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you want
>>> as well good deal many more PIC references as non-trapping.
>>>
>>
>> Yes, I like this idea.
>>
>> I tried to compile openssl with -m32 -fpic as an example, and counted
>> how often the mem[pic+const] is hit: that was 2353 times, all kind of
>> object refs.
>>
>> Then I tried your idea, and only 54 unhandled pic refs remained, all of
>> them looking like this:
>>
>> (plus:SI (reg:SI 107)
>>     (const:SI (plus:SI (unspec:SI [
>>                     (symbol_ref:SI ("bf_init") [flags 0x2] <var_decl
>> 0x2ac00f7bac60 bf_init>)
>>                 ] UNSPEC_GOTOFF)
>>             (const_int 4164 [0x1044]))))
>>
>> I believe that is a negligible fall out from such a big code base.
>>
>> Although the pic references do no longer reach the
>> SYMBOL_REF_FUNCTION_P in this version of the patch, I still see
>> that happening without -fpic option, so I left it as is.
>>
>>
>> Attached is the new version of my patch.
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.

[PING**3][PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

Ping...

I attached the patch again for your reference.
Is it OK for trunk?

Thanks
Bernd.

On 06/01/17 17:59, Bernd Edlinger wrote:
> Ping...
> 
> On 05/12/17 18:48, Bernd Edlinger wrote:
>> Ping...
>>
>> On 04/29/17 09:06, Bernd Edlinger wrote:
>>> On 04/28/17 20:46, Jeff Law wrote:
>>>> On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>>>>>
>>>>>
>>>>> Yes I agree, that is probably not worth it.  So I could try to remove
>>>>> the special handling of PIC+const and see what happens.
>>>>>
>>>>> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
>>>>> like to keep: It happens quite often, already w/o -fpic that call
>>>>> statements are using SYMBOL_REFs to ordinary (not weak) function
>>>>> symbols, and may_trap returns 1 for these call statements wihch is 
>>>>> IMHO
>>>>> wrong.
>>>> Hmm, thinking more about this, wasn't the original case a PIC 
>>>> referrence
>>>> for something like &x[BIGNUM].
>>>>
>>>> Perhaps we could consider a PIC reference without other arithmetic as
>>>> safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you 
>>>> want
>>>> as well good deal many more PIC references as non-trapping.
>>>>
>>>
>>> Yes, I like this idea.
>>>
>>> I tried to compile openssl with -m32 -fpic as an example, and counted
>>> how often the mem[pic+const] is hit: that was 2353 times, all kind of
>>> object refs.
>>>
>>> Then I tried your idea, and only 54 unhandled pic refs remained, all of
>>> them looking like this:
>>>
>>> (plus:SI (reg:SI 107)
>>>     (const:SI (plus:SI (unspec:SI [
>>>                     (symbol_ref:SI ("bf_init") [flags 0x2] <var_decl
>>> 0x2ac00f7bac60 bf_init>)
>>>                 ] UNSPEC_GOTOFF)
>>>             (const_int 4164 [0x1044]))))
>>>
>>> I believe that is a negligible fall out from such a big code base.
>>>
>>> Although the pic references do no longer reach the
>>> SYMBOL_REF_FUNCTION_P in this version of the patch, I still see
>>> that happening without -fpic option, so I left it as is.
>>>
>>>
>>> Attached is the new version of my patch.
>>>
>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>> Is it OK for trunk?
>>>
>>>
>>> Thanks
>>> Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr79286.diff --]
[-- Type: text/x-patch; name="patch-pr79286.diff", Size: 1905 bytes --]

2017-04-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        rtl-optimizatoin/79286
        * ira.c (update_equiv_regs): Revert to using may_trap_or_fault_p again.
        * rtlanal.c (rtx_addr_can_trap_p_1): SYMBOL_REF_FUNCTION_P can never
	trap.  PIC register plus a const unspec without offset can never trap.

Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 247397)
+++ gcc/ira.c	(working copy)
@@ -3551,7 +3551,8 @@ update_equiv_regs (void)
 	  if (DF_REG_DEF_COUNT (regno) == 1
 	      && note
 	      && !rtx_varies_p (XEXP (note, 0), 0)
-	      && def_dominates_uses (regno))
+	      && (!may_trap_or_fault_p (XEXP (note, 0))
+		  || def_dominates_uses (regno)))
 	    {
 	      rtx note_value = XEXP (note, 0);
 	      remove_note (insn, note);
Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 247397)
+++ gcc/rtlanal.c	(working copy)
@@ -485,7 +485,7 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST_WIDE_INT
     case SYMBOL_REF:
       if (SYMBOL_REF_WEAK (x))
 	return 1;
-      if (!CONSTANT_POOL_ADDRESS_P (x))
+      if (!CONSTANT_POOL_ADDRESS_P (x) && !SYMBOL_REF_FUNCTION_P (x))
 	{
 	  tree decl;
 	  HOST_WIDE_INT decl_size;
@@ -644,8 +644,11 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST_WIDE_INT
 
     case PLUS:
       /* An address is assumed not to trap if:
-         - it is the pic register plus a constant.  */
-      if (XEXP (x, 0) == pic_offset_table_rtx && CONSTANT_P (XEXP (x, 1)))
+	 - it is the pic register plus a const unspec without offset.  */
+      if (XEXP (x, 0) == pic_offset_table_rtx
+	  && GET_CODE (XEXP (x, 1)) == CONST
+	  && GET_CODE (XEXP (XEXP (x, 1), 0)) == UNSPEC
+	  && offset == 0)
 	return 0;
 
       /* - or it is an address that can't trap plus a constant integer.  */

Re: [PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Jeff Law]

On 04/29/2017 01:06 AM, Bernd Edlinger wrote:
> On 04/28/17 20:46, Jeff Law wrote:
>> On 04/28/2017 11:27 AM, Bernd Edlinger wrote:
>>> Yes I agree, that is probably not worth it.  So I could try to remove
>>> the special handling of PIC+const and see what happens.
>>>
>>> However the SYMBOL_REF_FUNCTION_P is another story, that part I would
>>> like to keep: It happens quite often, already w/o -fpic that call
>>> statements are using SYMBOL_REFs to ordinary (not weak) function
>>> symbols, and may_trap returns 1 for these call statements wihch is IMHO
>>> wrong.
>> Hmm, thinking more about this, wasn't the original case a PIC referrence
>> for something like &x[BIGNUM].
>>
>> Perhaps we could consider a PIC reference without other arithmetic as
>> safe.  That would likely pick up the SYMBOL_REF_FUNCTION_P case you want
>> as well good deal many more PIC references as non-trapping.
>>
> Yes, I like this idea.
> 
> I tried to compile openssl with -m32 -fpic as an example, and counted
> how often the mem[pic+const] is hit: that was 2353 times, all kind of
> object refs.
> 
> Then I tried your idea, and only 54 unhandled pic refs remained, all of
> them looking like this:
> 
> (plus:SI (reg:SI 107)
>      (const:SI (plus:SI (unspec:SI [
>                      (symbol_ref:SI ("bf_init") [flags 0x2] <var_decl 
> 0x2ac00f7bac60 bf_init>)
>                  ] UNSPEC_GOTOFF)
>              (const_int 4164 [0x1044]))))
> 
> I believe that is a negligible fall out from such a big code base.
> 
> Although the pic references do no longer reach the
> SYMBOL_REF_FUNCTION_P in this version of the patch, I still see
> that happening without -fpic option, so I left it as is.
> 
> 
> Attached is the new version of my patch.
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-pr79286.diff
> 
> 
> 2017-04-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
>         rtl-optimizatoin/79286
>         * ira.c (update_equiv_regs): Revert to using may_tap_p again.
>         * rtlanal.c (rtx_addr_can_trap_p_1): SYMBOL_REF_FUNCTION_P can never
> 	trap.  PIC register plus a const unspec without offset can never trap.
OK.  Sorry for the delay.  I've been swamped.

jeff

[PATCH][ PR rtl-optimization/79286] Drop may_trap_p exception to testing dominance in update_equiv_regs

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]


Per a discussion with Alan...

update_equiv_regs now tests that the insn which creates an equivalence 
dominates its uses.  With one exception, if may_trap_p returns false, we 
don't require dominance.

may_trap_p unfortunately returns false for PIC references in cases where 
they can clearly trap (as seen with the reduced testcase for this BZ in 
PIC mode).  I'm not really up for pulling on that thread right now.

Instead we just drop the may_trap_p exception and always verify 
dominance.  This fixes 79286 when generating PIC (and thus ought to fix 
the i686 darwin issues.

Bootstrapped and regression tested on i686-pc-linux-gnu.  Also hand 
verified the reduced testcase works with PIC.

Installing on the trunk.

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1162 bytes --]

commit 3a05b44836c9576b33101ed7c381ad21aa5f1581
Author: law <law@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Feb 24 15:36:10 2017 +0000

    	PR rtl-optimizatoin/79286
    	* ira.c (update_equiv_regs): Drop may_trap_p exception to
    	dominance test.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@245714 138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 22964c8..1239c4a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2017-02-24  Jeff Law  <law@redhat.com>
+
+	PR rtl-optimizatoin/79286
+	* ira.c (update_equiv_regs): Drop may_trap_p exception to
+	dominance test.
+
 2017-02-24  Richard Biener  <rguenther@suse.de>
 
 	PR tree-optimization/79389
diff --git a/gcc/ira.c b/gcc/ira.c
index 6fb8aaf..b41c480 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -3551,8 +3551,7 @@ update_equiv_regs (void)
 	  if (DF_REG_DEF_COUNT (regno) == 1
 	      && note
 	      && !rtx_varies_p (XEXP (note, 0), 0)
-	      && (!may_trap_p (XEXP (note, 0))
-		  || def_dominates_uses (regno)))
+	      && def_dominates_uses (regno))
 	    {
 	      rtx note_value = XEXP (note, 0);
 	      remove_note (insn, note);