On Tue, Jun 14, 2011 at 5:47 PM, H.J. Lu <hjl.to...@gmail.com> wrote: > 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. >
Sorry about the ia32 test failure. Looking into it now. -Easwaran