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
> >------------------------------------------------------------------------------
Doing More with Less: The Next Generation Virtual Desktop
What are the key obstacles that have prevented many mid-market businesses
from deploying virtual desktops? How do next-generation virtual desktops
provide companies an easier-to-deploy, easier-to-manage and more affordable
virtual desktop model.http://www.accelacomm.com/jaw/sfnl/114/51426474/
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
