Sorry guys, been very busy lately and I won't have time to go through this big change. Gautam, if you are fine with the changes (or whatever after your requested change), please make the call. Thx! Sun
On Mon, Sep 12, 2011 at 1:36 PM, Gautam Chakrabarti <gautam.c...@yahoo.com>wrote: > Sun: You had said you would also review this change, did you get a > chance? > Doug: Your attached change also includes an extra ipa_option.cxx change on > IPA output file size, please remove this change. > > I think I missed this in my earlier review. But for functions like > find_dir_path_in_list() > and find_fn_in_list() that take in a char*, it would be good to instead > take in "const char*". Similarly dir_list (and maybe some other > vectors/maps) could be a vector of "const char*". > > Otherwise looks fine to me. > > Thanks, > Gautam > > > *From:* "Gilmore, Doug" <doug.gilm...@amd.com> > *To:* Gautam Chakrabarti <gautam.c...@yahoo.com>; " > open64-devel@lists.sourceforge.net" <open64-devel@lists.sourceforge.net> > *Sent:* Friday, August 19, 2011 4:51 PM > *Subject:* RE: [Open64-devel] review request IPA global file table > implementation -- bug 675 > > > From: Gautam Chakrabarti [mailto:gautam.c...@yahoo.com] > > Sent: Monday, June 13, 2011 1:31 AM > > To: open64-devel@lists.sourceforge.net; Gilmore, Doug > > Subject: Re: [Open64-devel] review request IPA global file table > implementation -- bug 675 > > > > Hi Doug, > > > > Below are my comments. > > > > 1. I assume the copyright notices need to say 2011 instead of 2010. > I checked on this, the convention here has been to use copyright dates > associated with the commits to our internal repository. I have added > 2011 dates for the changes made in 2011. > > > > 2. While updating the stmt srcpos in IP_READ_fix_tree(), when mapped > > file number is 0, I am not sure if isrcpos should be reset to > > 0. Because this removes whatever file/line # the stmt had. > I got rid of this and added an assertion here and in other places, > since by construction the mapping from the local file tabel to global > should never produce a zero value. > > > > 3. Should output_queue::build_global_filelists also be under #ifndef > > _LIGHTWEIGHT_INLINER? > Fixed. > > > > 4. I see this code: > > > > + else { // !DST_IS_declaration && !DST_IS_memdef ==> DST_SUBPR_DEF > > > > Maybe there should be an assert to this effect. > The comments here were a bit confusing. I got rid of them and added > a comment that refers to the definition of the type DST_SUBPROGRAM > (in dwarf_DST.h). Given that the logic here matches that noted in > the comments in the field definitions in the type, there is no > need for assertions. > > > > 5. I think the strlen() checks before each strcmp() check are not > > required, and increase overhead. > Gone. > > > > 6. I don't see this in our coding guidelines, but I have seen it > > elsewhere, to prefer preincrements. This tends to be more > > efficient when an iterator or something more complicated is being > > incremented. An example where this should be used is in > > find_dir_path_in_list(). There are several other places too that > > currently use postincrement. > Yes that makes sense, I changed the "standalone" increments to be > pre-increments instead of post-increments. > > > > 7. In IPC_merge_DSTs(), I see 2 consecutive calls to > > fix_up_dst_srcpos(), both of which look identical. > How did I miss that? Gone. > > > > This is a pretty big change, so it would be good to have another > > local gatekeeper review it or a global gatekeeper sign off on it, > > once the above are resolved. > > > > Gautam > Thanks, and sorry for the delay in getting version 2 of the patch out > (see attached file). I'll be out for several weeks so there is no > hurry for you (and hopefully others) to review the updated version of > this patch. > > Doug > > > > > From: Gilmore, Doug <doug.gilm...@amd.com> > > > Subject: [Open64-devel] review request IPA global file table > implementation -- bug 675 > > > To: "open64-devel@lists.sourceforge.net" < > open64-devel@lists.sourceforge.net> > > > Date: Thursday, May 26, 2011, 12:02 AM > > > Attached is a patch that fixes bug 675 (line numbers of inlined > routines > > > is broken by IPA). > > > > > > The fix is basically described in a comment attached to the bug (which > I have > > > cleaned up a bit): > > > > > > Currently line number information in WHIRL statements for code being > inlined is > > > being replaced with the line number information associated with the > call (see > > > note * below). > > > > > > The fix is to have IPA generate a global file table along with maps > for each > > > input WHIRL file that maps an index into the input file table to an > index into > > > the global file table. > > > > > > Then during processing done IP_READ_fix_tree the file index field in > the > > > SRCPOS record in each WHIRL statement node can be changed to the > index > > > associated with global file table using the per WHIRL input object > file map. > > > > > > In output_queue::flush() the current state of the global file table > is > > > written to the current .I file. Note that the processing to do this > is done > > > by a newly added routine copy_DST_Type(). Also the routine > > > merge_directories_and_files(), which AFAICS has always generated > bogus file > > > tables, is no longer called and has been removed. > > > > > > * Note that code IPO_INLINE::Process_Op_Code was added with the PSC 1.3 > merge, it > > > was a work around to prevent assembly errors when out-of-ranges file > indices > > > were detected in .loc assembly directives (if you take out this work > around > > > in the current compiler the IPA compile of SPEC xalancbmk will likely > fail). > > > > > > This work was done by an engineer at AMD who is no longer working on > the > > > project, so I am taking over the maintenance of the change. > > > > > > Two minor issues I would like to attend to: > > > > > > 1) Make the array indices all start at 1 to eliminate the need of > adding > > > or subtracting 1 when values are inserted from one table to another. > > > > > > 2) Maps should be used to detect whether strings in the local > file/directory > > > tables already exists in the global table (this is currently done via > > > a linear search). > > > > > > Though I think these are minor issues that can be attended to later. > We have > > > been running with this code since late last year and it really a > worthwhile > > > change to incorporate, since the current implementation produces > useless line > > > number information in most situations when compiling with IPA. If you > are > > > analyzing the performance of IPA compiled code, this change is really > > > worthwhile. > > > > > > Note that I attached files to the bug report for testing this change, > using > > > the gdb test suite harness. > > > > > > One final note, even though with this change correct line/file table > > > information is being generated, on older Linux systems the `info line' > > > command in GDB may not be able to determine source location associated > with > > > a text address (this works on newer systems). > > > > > > For more information on this problem see: > > > > > > https://bugs.open64.net/show_bug.cgi?id=781 > > > > > > Could a gatekeeper review this change when they have the chance? > > > > > > Thanks, > > > > > > Doug > > > > > >
------------------------------------------------------------------------------ BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA Learn about the latest advances in developing for the BlackBerry® mobile platform with sessions, labs & more. See new tools and technologies. Register for BlackBerry® DevCon today! http://p.sf.net/sfu/rim-devcon-copy1
_______________________________________________ Open64-devel mailing list Open64-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/open64-devel
