diff mbox

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

Message ID AM4PR0701MB216261D19300246ACE8F85BEE41C0@AM4PR0701MB2162.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger April 23, 2017, 11:54 a.m. UTC 
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.

Comments

Jeff Law April 28, 2017, 4:48 p.m. UTC | #1 
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
Bernd Edlinger April 28, 2017, 5:27 p.m. UTC | #2 
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.
Jeff Law April 28, 2017, 6:46 p.m. UTC | #3 
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
Bernd Edlinger April 28, 2017, 7:14 p.m. UTC | #4 
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.
Bernd Edlinger April 28, 2017, 7:46 p.m. UTC | #5 
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.
diff mbox

Patch

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))