Re: Remove noce_mem_write_may_trap_or_fault_p in ifcvt

[Jeff Law <law@redhat.com>]

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