On Fri, 4 Nov 2016, Jakub Jelinek wrote:
> Hi!
>
> One of the common reasons why nonoverlapping_memrefs_p is called with
> MEM_EXPRs that are FUNCTION_DECLs and don't have DECL_RTL set is that
> DSE considers all calls to be memory reads from what is being called.
>
> The following patch avoids that. Of course the question is if we want
> to support something like:
> char var[64];
> ...
> var[0] = '0xe8';
> var[1] = ...;
> (void (*fn) (void) = (void (*) (void)) &var[0];
> fn ();
> without any kind of barrier, or not.
> If we do, then the patch below might break it by DSEing the stores.
> So, do we want the patch as is, or just limit it to direct function calls
> (call (mem (symbol_ref))), or only those where the symbol is actually
> a FUNCTION_DECL? Or do we want to consider arbitrary pointers possibly
> writing into function function text?
I'm not sure the current code handled this correctly? At least I
see nothing on the GIMPLE level that would disallow the DSE:
int foo ()
{
char a[256];
a[0] = 'a';
a[1] = 'b';
int __attribute__((const)) (*fn) (void) = (int (*) (void)) &a[0];
return fn ();
}
even with pure or regular fn this gets DSEd, the call is appearantly
not an escape point for &a.
That is, for DSE it would only matter for locals and/or const
function calls (if stores after the call make the earlier appear dead).
We did have bugs like PR70128 fixed though (but that was "global memory").
It might be still broken if the patched function is const and we
unpatch right after calling...
So to answer, yes, I think we want to treat this conservatively
(but we may have existing bugs here).
> In any case, the second hunk fixes an important DSE bug that just got
> revealed by the former change.
Do you have a testcase for the (wrong-code?) bug?
> 2016-11-04 Jakub Jelinek <ja...@redhat.com>
>
> PR target/77834
> * dse.c (check_mem_read_use): For CALL with MEM as first operand,
> ignore that MEM, but recurse on the MEM's operand if not SYMBOL_REF.
> (dse_step5): Call scan_reads even if just insn_info->frame_read.
> Improve and fix dump file messages.
>
> --- gcc/dse.c.jj 2016-11-03 15:52:12.521965058 +0100
> +++ gcc/dse.c 2016-11-04 09:42:27.640098125 +0100
> @@ -2159,6 +2159,19 @@ check_mem_read_use (rtx *loc, void *data
> rtx *loc = *iter;
> if (MEM_P (*loc))
> check_mem_read_rtx (loc, (bb_info_t) data);
> + else if (GET_CODE (*loc) == CALL && MEM_P (XEXP (*loc, 0)))
> + {
> + /* Ignore the MEM in first operand of CALL, that isn't
> + any kind of read of the function text memory. */
> + iter.skip_subrtxes ();
> + /* But for indirect function calls consider the MEM
> + containing the function pointer if any. Avoid recursing
> + in the most common case. */
> + if (GET_CODE (XEXP (XEXP (*loc, 0), 0)) != SYMBOL_REF)
> + check_mem_read_use (&XEXP (XEXP (*loc, 0), 0), data);
> + if (!CONST_INT_P (XEXP (*loc, 1)))
> + check_mem_read_use (&XEXP (*loc, 1), data);
> + }
> }
> }
>
> @@ -3298,12 +3311,19 @@ dse_step5 (void)
> bitmap_clear (v);
> }
> else if (insn_info->read_rec
> - || insn_info->non_frame_wild_read)
> + || insn_info->non_frame_wild_read
> + || insn_info->frame_read)
> {
> - if (dump_file && !insn_info->non_frame_wild_read)
> - fprintf (dump_file, "regular read\n");
> - else if (dump_file && (dump_flags & TDF_DETAILS))
> - fprintf (dump_file, "non-frame wild read\n");
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + if (!insn_info->non_frame_wild_read
> + && !insn_info->frame_read)
> + fprintf (dump_file, "regular read\n");
> + if (insn_info->non_frame_wild_read)
> + fprintf (dump_file, "non-frame wild read\n");
> + if (insn_info->frame_read)
> + fprintf (dump_file, "frame read\n");
> + }
> scan_reads (insn_info, v, NULL);
> }
> }
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
