On Thu, May 9, 2013 at 2:42 PM, Diego Novillo <dnovi...@google.com> wrote:
> On 2013-05-08 01:13 , Teresa Johnson wrote:
>>
>> Somehow Rietveld didn't upload the patch properly. I've attached the
>> patch to this email instead. Here is the description:
>
>
> Rietveld has turned out to be far less useful that I had hoped. If you are
> running ubuntu precise, the upload script is having some bad interaction
> with the server, which makes it to constantly reject your password.
>
> I do not recommend using Rietveld anymore. I don't really have the cycles
> to invest in fixing the various usability warts we've found. Sorry.
Thanks for the note. The main reason I have tried to keep using
Rietveld is that it sends out the patch inline in the email with the
formatting preserved. I have found that cut-n-paste into a gmail
window messes up the spacing. Do you know of a good way to work around
this issue?
>
>
>> -static void
>> +void
>> emit_barrier_after_bb (basic_block bb)
>> {
>> rtx barrier = emit_barrier_after (BB_END (bb));
>> - BB_FOOTER (bb) = unlink_insn_chain (barrier, barrier);
>> + if (current_ir_type () == IR_RTL_CFGLAYOUT)
>> + BB_FOOTER (bb) = unlink_insn_chain (barrier, barrier);
>
>
> What if the current IR is not RTL? Should we fail here? It doesn't seem
> like it makes sense to call this from gimple, for instance.
This is called only from bb-reorder and cfgrtl, so we should only be
in IR_RTL, I can add an assert to this effect.
More on this change when I respond to Steven's comments.
>
>
>> + several different possibilities. One is that there are edge weight
>> insanities
>> + due to optimization phases that do not properly update basic block
>> profile
>> + counts. The second is that the entry of the function may not be hot,
>> because
>> + it is entered fewer times than the number of profile training runs,
>> but there
>> + is a loop inside the function that causes blocks within the function
>> to be
>> + above the threshold for hotness. */
>> + if (cold_bb_count)
>> + {
>> + bool dom_calculated_here = !dom_info_available_p (CDI_DOMINATORS);
>> +
>
>
> Move this out into its own function?
Will do.
>
>> + if (dom_calculated_here)
>> + calculate_dominance_info (CDI_DOMINATORS);
>> +
>> + /* Keep examining hot bbs until we have either checked them all, or
>> + re-marked all cold bbs hot. */
>> + while (! bbs_in_hot_partition.is_empty ()
>> + && cold_bb_count)
>> + {
>> + basic_block dom_bb;
>> +
>> + bb = bbs_in_hot_partition.pop ();
>> + dom_bb = get_immediate_dominator (CDI_DOMINATORS, bb);
>> +
>> + /* If bb's immediate dominator is also hot then it is ok. */
>> + if (BB_PARTITION (dom_bb) != BB_COLD_PARTITION)
>> + continue;
>> +
>> + /* We have a hot bb with an immediate dominator that is cold.
>> + The dominator needs to be re-marked to hot. */
>
>
> s/to hot/hot/
ok. Actually, I think s/to hot/as hot/ might sound better.
>
>> Index: cfgrtl.c
>> ===================================================================
>> --- cfgrtl.c (revision 198686)
>> +++ cfgrtl.c (working copy)
>> @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see
>> #include "tree.h"
>> #include "hard-reg-set.h"
>> #include "basic-block.h"
>> +#include "bb-reorder.h"
>
>
> You may need to modify Makefile.in to declare this new dependency.
>
>> +/* Called when edge E has been redirected to a new destination,
>> + in order to update the region crossing flag on the edge and
>> + jump. */
>> +
>> +static void
>> +fixup_partition_crossing (edge e, basic_block target)
>> +{
>> + rtx note;
>> +
>> + gcc_assert (e->dest == target);
>
>
> Then, why not just take a single argument E?
Good idea, will do.
>
>> +fixup_bb_partition (basic_block bb)
>> +{
>> + edge e;
>> + edge_iterator ei;
>> +
>> + /* Now need to make bb's pred edges non-region crossing. */
>
>
> This is hard to parse.
Ok, how about:
/* Ensure edges to bb reflect its new partition assignment with the appropriate
region-crossing flag setting. */
>
>> + /* Delete any blocks that became unreachable and weren't
>> + already cleaned up, for example during edge forwarding
>> + and convert_jumps_to_returns. This will expose more
>> + opportunities for fixing the partition boundaries here.
>> + Also, the calculation of the dominance graph during verification
>> + will assert if there are unreachable nodes. */
>> + delete_unreachable_blocks ();
>
>
> Why not just schedule a CFG cleanup as a prerequisite to this pass?
Which pass? This is called right after we try to optimize the cfg
during cleanup_cfg, which is invoked numerous places. try_optimize_cfg
performs a number of cfg optimizations, some of which can create
unreachable blocks. I found it was much easier to clean this up in one
pass at the end rather that try to detect and fix this up
incrementally.
>
>
> A minor formatting nit. References to locals and function arguments should
> be done in capitals.
Ok, will clean this up.
Thanks!
Teresa
>
>
> Diego.
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
