Thanks for the comments.
Hopefully I'll have a new version of the patch ready for review in the near
future.
Doug
From: Sun Chan [mailto:sun.c...@gmail.com]
Sent: Monday, June 13, 2011 10:42 AM
To: Gautam Chakrabarti
Cc: open64-devel@lists.sourceforge.net; Gilmore, Doug
Subject: Re: [Open64-devel] review request IPA global file table implementation
-- bug 675
yes, we've also concluded that pre-increment produces better code in general.
I will try to look at that in the next couple of days
Sun
On Mon, Jun 13, 2011 at 1:30 AM, Gautam Chakrabarti
<gautam.c...@yahoo.com<mailto:gautam.c...@yahoo.com>> wrote:
Hi Doug,
Below are my comments.
1. I assume the copyright notices need to say 2011 instead of 2010.
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.
3. Should output_queue::build_global_filelists also be under #ifndef
_LIGHTWEIGHT_INLINER?
4. I see this code:
+ else { // !DST_IS_declaration && !DST_IS_memdef ==> DST_SUBPR_DEF
Maybe there should be an assert to this effect.
5. I think the strlen() checks before each strcmp() check are not required, and
increase overhead.
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.
7. In IPC_merge_DSTs(), I see 2 consecutive calls to fix_up_dst_srcpos(), both
of which look identical.
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
--- On Thu, 5/26/11, Gilmore, Doug
<doug.gilm...@amd.com<mailto:doug.gilm...@amd.com>> wrote:
From: Gilmore, Doug <doug.gilm...@amd.com<mailto:doug.gilm...@amd.com>>
Subject: [Open64-devel] review request IPA global file table implementation --
bug 675
To:
"open64-devel@lists.sourceforge.net<mailto:open64-devel@lists.sourceforge.net>"
<open64-devel@lists.sourceforge.net<mailto: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
-----Inline Attachment Follows-----
------------------------------------------------------------------------------
vRanger cuts backup time in half-while increasing security.
With the market-leading solution for virtual backup and recovery,
you get blazing-fast, flexible, and affordable data protection.
Download your free trial now.
http://p.sf.net/sfu/quest-d2dcopy1
-----Inline Attachment Follows-----
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net<http://us.mc380.mail.yahoo.com/mc/compose?to=Open64-devel@lists.sourceforge.net>
https://lists.sourceforge.net/lists/listinfo/open64-devel
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net<mailto:Open64-devel@lists.sourceforge.net>
https://lists.sourceforge.net/lists/listinfo/open64-devel
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
