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
<[email protected]>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 <[email protected]>* wrote:
>
>
> From: Gilmore, Doug <[email protected]>
> Subject: [Open64-devel] review request IPA global file table implementation
> -- bug 675
> To: "[email protected]" <
> [email protected]>
> 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
> [email protected]<http://us.mc380.mail.yahoo.com/mc/[email protected]>
> 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
> [email protected]
> 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/open64-devel
