IA-64 speculation patches have bad impact on ARM

[Maxim Kuvyrkov]

Daniel Jacobowitz wrote:
> Hi Maxim and Vlad,
> 
> I just tracked an ICE while building glibc for ARM to this patch,
> which introduced --param max-sched-extend-regions-iters with a default
> of two:
> 
>   http://gcc.gnu.org/ml/gcc-patches/2006-03/msg00998.html

...

> The register variables and their initializations get hoisted all the way out
> of the first if.  On ia64, with a million execution units to spare and a
> fat pipeline, this may make sense.  On targets with a simpler execution
> model, though, it's pretty awful.  If the condition (which we have no
> information on the likelihood of) is false, we've added lots of cycles for
> no gain.  It's not like the scheduler was filling holes; the initializations
> were scheduled as early as possible because they had no dependencies.
> 
> With the parameter turned back down to one, the testcase compiles, and the
> code looks sensible again.  No, I wasn't able to work out why profiling was
> necessary to trigger this problem; I suspect it makes some register
> unavailable, but I'm not sure which.  I didn't look into that further.
> 
> What's your opinion?  We could easily change the default of the parameter
> for ARM, but I assume there are other affected targets.  I don't know if we
> need the extended region scheduling to be smarter, or if it should simply be
> turned off for some targets.
> 

Hi Daniel!

Sorry for the delay, I needed time to investigate the cause of the problem.

The real problem lies in the computation of the instruction priorities. 
  ARM has fairly simple scheduling model: on each cycle insn standing 
first in the ready list gets scheduled.  This behavior puts *all* the 
responsibility for the resulting schedule on how the ready list is 
arranged.  The main decision factor of the sorting of the ready list is 
INSN_PRIORITY.

Instructions in inner 'if' get somewhat greater priority then the 
instructions from the dominator block and hence get hoisted from their 
original block.  The good solution for this case would be more precise 
evaluation of the insn priorities.  This includes transformation of the 
insn priority from the region-scope to the block-scope value: e.g. in 
this case, while scheduling the first block, the priorities of the insns 
from the 'if'-block will be multiplied by probability of the 
'then'-branch and, therefore, will be significantly lower than the 
priority of the insns from the current block.  I've started to implement 
this idea some time ago, but never finished :(

Anyway, this work is for stage 1 or 2 and for now I propose following 
fix: implement targetm.sched.reorder hook so that it will ensure that if 
there is an insn from the current block in the ready list, then insn 
from the other block won't stand first in the line (and, therefore, 
won't be chosen for schedule).  I feel that this will be what you are 
calling 'filling holes'.  Please find an example patch attached (arm.patch).

While debugging the testcase I found two somewhat unrelated bugs in the 
handling of INSN_PRIORITY: first one is in the haifa-sched.c: priority 
().  When insn has no forward dependencies its priority is set to its 
latency.  The bug occurs when insn has some deps and all of them get 
rejected by current_sched_info->contributes_to_priority () hook - in 
this case INSN_PRIORITY should also be initialized with insn latency, 
but present code misses that.

The second one is not as critical as the first one.  It is in 
haifa-sched.c: adjust_priority ().  This function plainly calls the 
targetm.sched_adjust_priority () hook when insn is being added to the 
ready list.  As I understand all targets assume this hook to be invoked 
once: after all priorities are are computed, but before insn is added to 
the ready list.  But for insns with no dependencies from the source 
blocks this hook can be called many times - therefore priorities of that 
insns can become sensibly inadequate.

The patch for these two small bugs is also attached 
(priority-bugs.patch) .  Is it ok for trunk?  If so I will repost it to 
gcc-patches list.


Best regards,
Maxim
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: arm.patch
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20060530/ef4a6629/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: priority-bugs.clg
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20060530/ef4a6629/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: priority-bugs.patch
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20060530/ef4a6629/attachment-0002.ksh>