Re: PATCH: New Optimization: Partitioning hot & cold basic blocks

[Richard Henderson <rth at redhat dot com>]

On Wed, Jan 28, 2004 at 03:01:14PM -0800, Caroline Tice wrote:
> I have incorporated all the suggestions into the patch...

No, you havn't.  You've ignored all of my objections.

 (1) You've done nothing to ensure that this code isn't used with
     targets that don't support it.  Which is virtually all of them.

 (2) It generates actively incorrect EH info.  This means that if
     you compile C++ with your switch, it will fail.

 (3) It generates actively incorrect debug info.  Debugging code
     with this switch enabled will be impossible.

> + #define LONG_COND_BRANCH_SIZE 2000000

 (4) This is absolutely the wrong way to do whatever it is you're
     trying to do with this.  I'm quite certain that decimal 2000000
     does not correspond to any powerpc bit field limitation, and is
     thus effectively chosen at random.

     What, then, is the difference between having ports define this
     value and having the generic code use UINT_MAX, or equivalent?

     Worse, this is the *size* of the instruction?  I think not.
     The size of the long branch is going to be 16 or 32 or something.
     The distance traveled may be quite large, but that's different.

If you want something like this to be accepted, you're going to have
to arrange for it to be portable to more than just powerpc and x86.
This means 

 (A1) If and only if the target defines some new magic pattern name,
      say "long_cond_branch", then you can translate conditional
      branches between partitions directly to the new form.

 (A2) Otherwise, if and only if the target defines some new magic
      pattern name, say "long_branch", then you can translate 
      conditional branches between partitions to a branch across
      branch, or branch to branch.

      Given that only a scant few of our targets can actually
      support long conditional branches, and almost all can only
      support long unconditional branches, I'm going to require
      that the long unconditional branch support be generic and
      not buried inside 30 target md files.

 (A3) Otherwise, the option should be rejected at the beginning of
      compilation, as we do with other features the target doesn't
      support.

 (B)  You *must* come up with a solution to the problem of incorrect
      exception handling info.  This is a matter of correctness.

 (C)  You *should* come up with a plan to address incorrect debug info.
      Since it's not a code correctness problem, I won't require that
      it be done at the same time as the main patch, but you can't
      ignore it entirely.

 (D)  There must be tests for this feature added to the testsuite.
      You should be able to use __builtin_expect to create exactly
      the partition desired.


r~