Message ID | AM4PR0701MB216261D19300246ACE8F85BEE41C0@AM4PR0701MB2162.eurprd07.prod.outlook.com |
---|---|
State | New |
Headers | show |
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
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.
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
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.
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.
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))