Re: using scratchpads to enhance RTL-level if-conversion: the new patch now passes bootstrap with the default BUILD_CONFIG [i.e. no stage2-to-stage3 comparison errors even with debugging info off in stage2 and on in stage3] AND passes "make check" testing with no new regressions

[Bernd Schmidt <bschmidt at redhat dot com>]

This is currently not really reviewable due to broken indentation, 
possibly due to whitespace damage from your mailer or not following 
coding guidelines. Please ensure your code is formatted the same way as 
all other code in gcc. I'll point out some of the problems, but please 
investigate
  https://www.gnu.org/prep/standards/html_node/Writing-C.html
and fix everything before resubmission. Also read through maybe one or 
two whole gcc source files to get an impression of how things should 
look like.

>  static bool
> -noce_mem_write_may_trap_or_fault_p (const_rtx mem)
> +noce_mem_write_may_trap_or_fault_considering_scratchpad_support_p
> +  (const_rtx mem, bool scratchpads_enabled)

Just no. The presence of the argument and its documentation is enough to 
show that scratchpad support is considered. We don't want identifiers 
taking up a whole line.

> +      if (optimize<2)
> +      {
> +        return FALSE;
> +      }

Lose braces around single statements.

> +          if
> (size_of_MEM_operand<=SCRATCHPADS_INCLUSIVE_MAX_SIZE_IN_BYTES)
> +          {
> +
> +        if (size_of_MEM_operand > size_of_biggest_scratchpad)
> +        {

Serious problems with the indentation. Check whether it's your mailer 
doing it or if this is a problem in your code. If it's the former, try 
using a text/plain attachment for sending patches. Also, identifiers too 
long.

> +        for
> +          (insn_to_maybe_duplicate = BB_HEAD (then_bb);
> +           insn_to_maybe_duplicate &&
> +             (insn_to_maybe_duplicate != insn_a) &&
> +             (insn_to_maybe_duplicate != BB_END (then_bb));

Use shorter identifiers (everywhere) to avoid having to use multiple 
lines. "cand", short for candidate, might be a good option here.
No need to use parentheses around != comparisons. Logical ops like && 
should start a line not end it.

> +           insn_to_maybe_duplicate=NEXT_INSN (insn_to_maybe_duplicate))

Spaces around operators.

> +        if (MEM_ADDR_SPACE (orig_x) !=
> +            MEM_ADDR_SPACE (biggest_scratchpad_rtx))

Again, operator should start a line.

> +        /* Abe`s note: do we need to do the following after getting
> +           a new pseudo-reg., as shown elsewhere in this file?
> +          if (max_regno < max_reg_num ())  max_regno = max_reg_num ();
> +        */

Avoid referencing yourself in the comments with things like "Abe's 
note". (Also, odd use of the backquote character there).

> +          } /* (size_of_MEM_operand <=
> +                     SCRATCHPADS_INCLUSIVE_MAX_SIZE_IN_BYTES) */

Lose such end comments.

> basic_block merge_bb,
>   static void
>   if_convert (bool after_combine)
>   {
> +
>     basic_block bb;
>     int pass;

Spurious whitespace change.

> +  biggest_scratchpad_rtx = 0; /* Reminder: an "rtx", therefore a
> pointer.  */

Comments go before a line, and this particular comment should just be 
removed as useless.

I'll point out one possible problem with the logic now (I'll wait for a 
properly formatted version before going in depth):

> +        if (size_of_MEM_operand > size_of_biggest_scratchpad)
> +        {
> +          size_of_biggest_scratchpad = size_of_MEM_operand;
> +          biggest_scratchpad_rtx = assign_stack_local
> +            (GET_MODE (orig_x), size_of_MEM_operand, 0);
> +            /* 0: align acc. to the machine mode indicated by
> +              "GET_MODE (orig_x)" */
> +          gcc_assert (biggest_scratchpad_rtx);
> +          scratchpads.safe_push (std::make_pair (biggest_scratchpad_rtx,
> +                             size_of_MEM_operand));
> +        }
> +

It looks like you're allocating extra stack slots, and later code can 
decide not to use them. This seems like it could be problematic.


Bernd