Re: [PATCH/RFC v2] PR68212: Improve Accounting of Block Frequencies During Loop Unrolling

[Bernd Schmidt <bschmidt at redhat dot com>]

I'm reading up on this stuff, but I'm probably still not the best person 
to review the actual frequency manipulation parts in this. There are a 
few things I can comment on, however.

The first question would be, have you looked at the rebuild_frequencies 
code in predict.c, and whether you can reuse that entirely or in part?

> +/* Return true iff BLOCK is considered to reside within the loop
> +   represented by LOOP_PTR. */
> +bool
> +in_loop_p (basic_block block, struct loop *loop_ptr)
> +{
> +  return (block->loop_father == loop_ptr)
> +    || flow_loop_nested_p (loop_ptr, block->loop_father);
> +}

As Bernard pointed out previously, use flow_bb_inside_loop_p() ? Also, 
formatting - multiline conditions should be wrapped in parentheses to 
ensure proper indentation.

> +  for (unsigned i = 0; i < loop_ptr->num_nodes; ++i)
> +    {
> +      bbs[i]->frequency = 0;
> +    }

Formatting. Please check all your patch for instances of this or the 
other formatting problems.

> +/* A list of block_ladder_rung structs is used to keep track of all
> +   the blocks visited in a depth-first recursive traversal of a control-flow
> +   graph.  This list is used to detect and prevent attempts to revisit
> +   a block that is already being visited in the recursive traversal. */
> +typedef struct block_ladder_rung {
> +  basic_block block;
> +  struct block_ladder_rung *lower_rung;
> +} *ladder_rung_p;

I had comments about this previously. I don't think what you're doing 
here prevents blocks entirely from being revisited, and there is no 
indication whether that is intentional. A worklist algorithm would still 
be preferrable because I expect it would be significantly more compact 
and easier to follow.

> +  free(loop_body);

Whitespace.

> +      change_in_edge_frequency =
> +	new_edge_frequency - original_edge_frequency;

The = goes at the start of the line.


Bernd