On Mon, May 2, 2011 at 8:37 PM, Jeff Law <l...@redhat.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 04/26/11 16:06, Easwaran Raman wrote:
>
>>
>>> You're right. The patch has correctness issues. It is not possible to
>>> simply not call add_wild_read because it also resets
>>> active_local_stores and frees read_recs. During the local phase it
>>> seems better to just treat calls as wild reads and reset
>>> active_local_stores and free read_recs. I've now refactored
>>> add_wild_read so that resetting active_local_stores and free read_recs
>>> are in separate methods that can be called on non-const/non-memset
>>> calls. In addition, I have added a non_frame_wild_read field in
>>> insn_info to mark non-const and non-memset calls.
>>
>>> I've attached the revised patch.
> Looking better. Just a few more things.
>
> Don't all locals escape if the callee has a static chain? Is that
> handled anywhere?
>
> Don't you still have the potential for wild reads in dse_step5_nospill
> (say from an asm)? if so, then the change can't be correct.
>
> Couldn't you just add a clause like this before the else-if?
>
> else if (insn_info->non_frame_wild_read)
> {
> if (dump_file)
> fprintf (dump_file, "non-frame wild read\n");
> scan_reads_nospill (insn_info, v, NULL);
> }
> else if (insn_info->read_rec)
> {
> /* Leave this clause unchanged */
> }
>
> Am I missing something?
I am not sure I understand the problem here. If there is a wild read
from asm, the instruction has the wild_read flag set. The if statement
checks if that flag is set and if so it clears the bitmap - which was
the original behavior. Originally, only if read_rec is non NULL you
need to recompute the kill set. Now, even if read_rec is NULL,
non_frame_wild_read could be set requiring the kill set to be
modified, which is what this patch does. In fact, isn't what you have
written above the equivalent to what is in the patch as '/* Leave this
clause unchanged */' is the same as
if (dump_file)
fprintf (dump_file, "regular read\n");
scan_reads_nospill (insn_info, v, NULL);
-Easwaran
> What's the purpose behind using unit64_t in the testcase? Somehow I
> suspect using int64_t means the test is unlikely not going to work
> across targets with different word sizes.
>
> Jeff
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iQEcBAEBAgAGBQJNv3iGAAoJEBRtltQi2kC7C8oH+gPi/6DvV2JkmitT3xEeUaW8
> szOHC0GysQkot/TiVlQQy6fiz61G49ii0mz68W4cfSntvuN7iaqBqVWpRKWIzvkx
> alk4U1snj9a2t9+9ZRTX4xm3quggTv+mUDK81a+DIS2zAf6i/HRXLvQbx6fhDOYD
> sqXqSkvCKqkH2pPHHYEqnBtS/cLFtAfZJe+JSx3u2oqL+sBFGLftdoV6yJzkShLS
> LpmYHMeDbzhdCtZTf15GQm3h/cBlyrChxjQsjJxLiXk5rrcDI/X+Pqi+cll21Gwg
> mlzMBi0iYToENl+7aO8DNGvXfliCEzQ7iEUyTctJiTDt3/RgVcaxgRJgqHZNSBI=
> =VOdm
> -----END PGP SIGNATURE-----
>
