| Message ID | 563CE17F.6090308@t-online.de |
|---|---|
| State | New |
| Headers | show |
On 11/06/2015 10:21 AM, Bernd Schmidt wrote: > The ifcvt scratchpad patch had some modifications to the function > noce_mem_write_may_trap_or_fault_p in ifcvt, but as far as I can tell, > that function itself makes no sense whatsoever. It returns true for > MEM_READONLY_P which is sensible, but then it also goes on an unreliable > search through the address, looking for SYMBOL_REFs that are in > decl_readonly_section. Needless to say, this won't ever find anything on > targets that don't allow symbolic addresses, and is not reliable even on > targets that do. The MEM_READONLY_P test must suffice; if there's a > reason why it doesn't, we'd need to figure out why and possibly disable > if-conversion of stores more thoroughly. > > As a sanity check I bootstrapped and tested the first of the two > attached patches, which changes the "return true" paths to > gcc_unreachable. Since that passed, I propose the second of the two > attached patches, which removes the function and replaces it with a > simpler check. Ok if that passes too? Given the name "..may_trap_or_fault_p" ISTM that its mode of operation should be to return true (the safe value) unless we can prove the write will not fault. The more cases we can prove true, the better AFAICT. The PLUS case looks totally wrong. Though it could possibly be made correct by looking for [sp,fp,ap] + offset addresses and verifying we're doing a mis-aligned write. We'd probably also need some kind of sensible verification that the offset isn't too large/small. The SYMBOL_REF case is interesting too. Can a write to a SYMBOL_REF that is not in a readonly section ever fault? Perhaps a weak symbol? Or a mis-aligned write? So I totally agree this code is bogus. The fact that we're not hitting those cases is disturbing though. You might look at BZ23567 and its associated test 20051104-1.c. Presumably those aren't hitting this path anymore, but why? Jeff
On 11/06/2015 07:39 PM, Jeff Law wrote: > Given the name "..may_trap_or_fault_p" ISTM that its mode of operation > should be to return true (the safe value) unless we can prove the write > will not fault. The more cases we can prove true, the better AFAICT. > > The PLUS case looks totally wrong. Though it could possibly be made > correct by looking for [sp,fp,ap] + offset addresses and verifying we're > doing a mis-aligned write. We'd probably also need some kind of > sensible verification that the offset isn't too large/small. I'm guessing this is already covered by the call to may_trap_or_fault_p. The only additional thing that this function tries to prove is that the mem isn't readonly. IMO either MEM_READONLY_P is sufficient for that (and my patches operate under that assumption), or it isn't sufficient and no amount of checking the address is going to make the function useful. Bernd
On 11/06/2015 11:44 AM, Bernd Schmidt wrote: > On 11/06/2015 07:39 PM, Jeff Law wrote: >> Given the name "..may_trap_or_fault_p" ISTM that its mode of operation >> should be to return true (the safe value) unless we can prove the write >> will not fault. The more cases we can prove true, the better AFAICT. >> >> The PLUS case looks totally wrong. Though it could possibly be made >> correct by looking for [sp,fp,ap] + offset addresses and verifying we're >> doing a mis-aligned write. We'd probably also need some kind of >> sensible verification that the offset isn't too large/small. > > I'm guessing this is already covered by the call to may_trap_or_fault_p. Yea, it looks like may_trap_or_fault_p tries to handle unaligned and wacky offsets for sp,fp,ap relative addresses. So maybe what noce_mem_write_may_trap_or_fault_p is really trying to do is take objects where may_trap_or_fault_p returns false, but which in fact may fault if we write that memory? ie, working around lameness in may_trap_or_fault_p not knowing the context (ie read vs write). > The only additional thing that this function tries to prove is that the > mem isn't readonly. IMO either MEM_READONLY_P is sufficient for that > (and my patches operate under that assumption), or it isn't sufficient > and no amount of checking the address is going to make the function useful. Right. I think we've come to agreement on what noce_mem_write_may_trap_or_fault_p is trying to to. The problem I have is I don't think we can assume that the lack of MEM_READONLY_P is sufficient to determine that a particular memory reference is not to readonly memory. ie, when MEM_READONLY_P is set, the object must be readonly, when it is not set, we know nothing. I think what we really want here is to fix may_trap_or_fault_p to be safe. It's returning "false" in cases where it really ought be returning "true". Then we just use may_trap_or_fault_p, dropping noce_mem_write_may_trap_or_fault_p. Jeff
On 11/06/2015 08:20 PM, Jeff Law wrote: > So maybe what noce_mem_write_may_trap_or_fault_p is really trying to do > is take objects where may_trap_or_fault_p returns false, but which in > fact may fault if we write that memory? ie, working around lameness in > may_trap_or_fault_p not knowing the context (ie read vs write). Yes exactly, that is the aim - detect extra possibilities for faults knowing that the context is a write. > I think what we really want here is to fix may_trap_or_fault_p to be > safe. It's returning "false" in cases where it really ought be > returning "true". Then we just use may_trap_or_fault_p, dropping > noce_mem_write_may_trap_or_fault_p. Well, I think if MEM_READONLY_P is insufficient (and I guess people could cast away const - bother), then the current noce_mem_write_may_trap_or_fault_p should be simplified to "return true;" and eliminated. We could try to special case stack accesses within a known limit later on. Maybe it doesn't even matter given that we call noce_can_store_speculate immediately after this test. Bernd
On 11/06/2015 12:30 PM, Bernd Schmidt wrote: > On 11/06/2015 08:20 PM, Jeff Law wrote: >> So maybe what noce_mem_write_may_trap_or_fault_p is really trying to do >> is take objects where may_trap_or_fault_p returns false, but which in >> fact may fault if we write that memory? ie, working around lameness in >> may_trap_or_fault_p not knowing the context (ie read vs write). > > Yes exactly, that is the aim - detect extra possibilities for faults > knowing that the context is a write. > >> I think what we really want here is to fix may_trap_or_fault_p to be >> safe. It's returning "false" in cases where it really ought be >> returning "true". Then we just use may_trap_or_fault_p, dropping >> noce_mem_write_may_trap_or_fault_p. > > Well, I think if MEM_READONLY_P is insufficient (and I guess people > could cast away const - bother), then the current > noce_mem_write_may_trap_or_fault_p should be simplified to "return > true;" and eliminated. We could try to special case stack accesses > within a known limit later on. > > Maybe it doesn't even matter given that we call noce_can_store_speculate > immediately after this test. may_trap_or_fault_p already has this kind of knowledge. It just doesn't know if the memory is a read or a write. Hence my suggestion we should fix may_trap_or_fault_p. Essentially we pass in the type of access, read, write, unknown (where unknown must essentially be treated as write). That gets passed down into the helpers where it's taken into account. Particularly in rtx_addr_can_trap_p_1. My concern with relying on MEM_READONLY_P and/or may_trap_or_fault_p is we know that may_trap_or_fault_p been called before in cases where it's returned false for readonly memory locations. Ripping out noce_mem_write_may_trap_or_fault_p without fixing may_trap_or_fault_p introduces a latent code code generation issue. Jeff
* ifcvt.c (noce_mem_write_may_trap_or_fault_p): Delete function. (noce_process_if_block): Don't call it, test MEM_READONLY_P and may_trap_or_fault_p instead. Index: gcc/ifcvt.c =================================================================== --- gcc/ifcvt.c (revision 229049) +++ gcc/ifcvt.c (working copy) @@ -2828,59 +2828,6 @@ noce_operand_ok (const_rtx op) return ! may_trap_p (op); } -/* Return true if a write into MEM may trap or fault. */ - -static bool -noce_mem_write_may_trap_or_fault_p (const_rtx mem) -{ - rtx addr; - - if (MEM_READONLY_P (mem)) - return true; - - if (may_trap_or_fault_p (mem)) - return true; - - addr = XEXP (mem, 0); - - /* Call target hook to avoid the effects of -fpic etc.... */ - addr = targetm.delegitimize_address (addr); - - while (addr) - switch (GET_CODE (addr)) - { - case CONST: - case PRE_DEC: - case PRE_INC: - case POST_DEC: - case POST_INC: - case POST_MODIFY: - addr = XEXP (addr, 0); - break; - case LO_SUM: - case PRE_MODIFY: - addr = XEXP (addr, 1); - break; - case PLUS: - if (CONST_INT_P (XEXP (addr, 1))) - addr = XEXP (addr, 0); - else - return false; - break; - case LABEL_REF: - return true; - case SYMBOL_REF: - if (SYMBOL_REF_DECL (addr) - && decl_readonly_section (SYMBOL_REF_DECL (addr), 0)) - return true; - return false; - default: - return false; - } - - return false; -} - /* Return whether we can use store speculation for MEM. TOP_BB is the basic block above the conditional block where we are considering doing the speculative store. We look for whether MEM is set @@ -3210,7 +3157,8 @@ noce_process_if_block (struct noce_if_in THEN and ELSE arms, then we can go ahead with the conversion; either the program is broken, or the condition is always false such that the other memory is selected. */ - if (noce_mem_write_may_trap_or_fault_p (orig_x)) + if (may_trap_or_fault_p (orig_x) + || MEM_READONLY_P (orig_x)) return FALSE; /* Avoid store speculation: given "if (...) x = a" where x is a