> 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 > >
bug675_v2.patch
Description: bug675_v2.patch
------------------------------------------------------------------------------ Get a FREE DOWNLOAD! and learn more about uberSVN rich system, user administration capabilities and model configuration. Take the hassle out of deploying and managing Subversion and the tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________ Open64-devel mailing list Open64-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/open64-devel
