Re: Patches Ping^2 (4.1 projects stage 1.1) SMS improvements

[Mostafa Hagog <MUSTAFA at il dot ibm dot com>]

Hi Roger,

Thanks for the review,
all the below patches passes bootstrap & regression testing on
powerpc-apple-darwing and i686-pc-linux-gnu. I will be addressing your
below comment and resubmitting the SMS patches. I will start (as you
suggested) by committing the fix to PR20177 to mainline and then to 4.0
branch.

Mostafa.

Roger Sayle <roger@eyesopen.com> wrote on 20/03/2005 17:21:55:

>
> Hi Mostafa,
>
> On Sun, 20 Mar 2005, Mostafa Hagog wrote:
> > 1. RTL loop versioning:
> > http://gcc.gnu.org/ml/gcc-patches/2005-03/msg00131.html
> >
> > 2. SMS improvements:
> > http://gcc.gnu.org/ml/gcc-patches/2005-03/msg01101.html
>
> Now, that we've slipped into stage 1.2, and these patches haven't
> been reviewed, I guess I should review them.  Firstly, it goes without
> saying that you probably know much more about Swing Modulo Scheduling
> than I do, indeed you've probably though more about it than most GCC
> maintainers, so I'll have to trust you that this is algorithmically
> the correct way to go, unless Zdenek or RTH have any objections.
>
> Firstly, it looks like you pinged the wrong RTL loop versioning patch,
> the above message contains loop_version_hook_4.patch was superseded
> in http://gcc.gnu.org/ml/gcc-patches/2005-03/msg00681.html which contains
> loop_version_hook_5.patch.
>
> There only appear to be two types of issues with these patches.  The
> first is that you introduce new functions without comments above them,
> describing what they do and explicitly listing the arguments they take
> and the result they return.
>
> lv_flush_pending_stmts
> cfg_hook_duplicate_loop_to_header_edge
> extract_cond_bb_edges
> lv_adjust_loop_header_phi
> lv_add_condition_to_bb
> ...
>
> For example,
>
> + /* Add COMP_RTX as a condition at end of COND_BB and */
> + static void
> + rtl_lv_add_condition_to_bb (basic_block first_head ATTRIBUTE_UNUSED,
> +              basic_block second_head,
> +              basic_block cond_bb, void *comp_rtx)
>
> The comment appears truncated and clearly isn't sufficient.
>
> and
>
> + /* We suppose that we have a conditional branch at end of the block.
*/
> + static void
> + rtl_extract_cond_bb_edges (basic_block b, edge *branch_edge,
> +             edge *fallthru_edge)
>
> Likewise.
>
>
> And similarly in your second SMS improvements patch:
> undo_generate_reg_moves
> free_undo_replace_buff
> ps_unschedule_note
> etc...
>
>
>
> The second class of problems is that despite all of this hard work there
> doesn't seem to be a single new test case for SMS!?  Even with your patch
> to resolve PR20177 which an ICE-on-valid that has a reduced testcase
> attached to the PR, you failed to add it to the testsuite to prevent
> problems from reappearing in the future.  Hence, at the *very* least you
> need to add test cases that catch the bugs that have needed to be fixed
> in your code. Better yet, you'd add a handful of testcases that ensure
> that your new  code gets executed (from a code coverage perspective), but
> ideally you'd add some assembler scans, tree-ssa-dump scans or execution
> tests to confirm that these transformations/optimizations are being
> performed, even if such testing isn't portable.
>
> Admitedly, your bootstrapping with -fmodulo-sched is a convincing testing
> policy, but its unlike passes enabled by default, it's unlikely that such
> bootstrapping with -fmodulo-sched will be performed regularly once these
> changes are in CVS.
>
>
> Finally, you also don't appear to have posted a follow-up message
> to "[PATCH] SMS improvements 2/2" confirming that regression tested
> completed on i686 and powerpc without problems.  The wording in
> contribute.html makes it extremely clear that you post to gcc-patches
> *after* you've finished testing.  This may be one reason these
> contributions have slipped through the cracks.  Generally, there's
> never a valid reason to post a patch until its ready (and I have to
> chuckle at the poor embarrassed fools that have to correct or withdraw
> patches after they've been submitted.  There are however a few patches
> that get approved and applied to CVS before they've finished testing,
> and when they break I don't chuckle).
>
>
> Given the timing how to proceed?...
>
> I suggest that you immediately apply the small set of PR20177 fixes to
> mainline and add it's testcase to the testsuite as a gcc.dg test with
> dg-options "-O2 -fmodulo-sched".  With 4.0 coming soon, it would be
> nice to have SPEC working with -fmodulo-sched there.  So testing the
> fix set independently on mainline for a day or two, then backporting to
> teh release branch seems reasonable.  For the remainder of these two
> patches, I'd recommend resubmitting (still as two separate patches) with
> updated commentary and perhaps some testcases.  Given these changes are
> mostly cosmetic the revised changes should be approved quickly.
>
> Given my intention to approve the revised patches once they are
submitted,
> now would be a good time for other interested maintainers to suggest a
> second opinion (or forever hold their peace :-).
>
>
> Mostafa, thanks again for these improvements.
>
> Roger
> --
>