On Tue, May 10, 2011 at 12:18 PM, Easwaran Raman <era...@google.com> wrote:
> On Tue, May 3, 2011 at 9:40 AM, Easwaran Raman <era...@google.com> wrote:
>> 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
>>
>
> Ping. I have changed the test case to use int and added another test
> case that shows DSE doesn't happen when the struct instance is
> volatile (wild_read gets set in that case)
>
>
One test failed on Linux/ia32:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49414
--
H.J.
