Steven Bosscher <stevenb....@gmail.com> writes:
> On Sun, Nov 25, 2012 at 6:13 PM, Richard Sandiford wrote:
>> Steven Bosscher writes:
>>> Hello,
>>>
>>> reorg.c:rare_destination() tries to determine whether a given insn is
>>> a likely destination of a branch. To make this determination, it walks
>>> the insns chain from insn and stops at barriers, labels, return insns,
>>> or if more than 10 jumps have been followed.
>>>
>>> The function is supposed to return 2 for noreturn calls, 1 for return,
>>> and 0 otherwise. But a quick experiment shows that the function
>>> doesn't work that way at all:
>>>
>>> 1. for all non-label, non-jump, non-barrier insns the function reaches
>>> the default case of the loop, which results in "return 1".
>>
>> I might be misunderstanding what you mean, but the structure is:
>>
>> for (; insn && !ANY_RETURN_P (insn); insn = next)
>> {
>> ...
>> next = NEXT_INSN (insn);
>> switch (GET_CODE (insn))
>> {
>> ...
>> default:
>> break;
>> }
>> }
>>
>> So we skip those instructions and go on to the next. The function
>> doesn't look quite as broken as all that.
>
> Right, I mis-read the break as exiting the for-loop, but of course it
> only exists the switch.
>
>
>> Still, I agree that returning 0 for all conditional jumps is a pretty
>> big hole that could make them seem more likely than they should:
>>
>>> 4. The combination of the above 3 points results in the fall-through
>>> path being predicted as a rare destination and the branch path as a
>>> more likely destination. It should be the other way around, if basic
>>> block reordering has done its job properly.
>>
>> and that REG_BB_PROB should be a more reliable indicator:
>>
>>> 5. the only case that does work is the no-return call case, where the
>>> function stops at a BARRIER which must be for a no-return call because
>>> the function doesn't scan past jump insn (it follows jumps instead).
>>> However, this case is already handled in mostly_true_jump when it
>>> looks at REG_BR_PROB (which is a more reliable source of branch
>>> probabilities for all other cases as well).
>>
>> But removing the function seems to put more weight on:
>>
>> /* Predict backward branches usually take, forward branches usually not.
>> If
>> we don't know whether this is forward or backward, assume the branch
>> will be taken, since most are. */
>>
>> which doesn't seem any more reliable.
>>
>> I think your point is that we
>> should assume most branches _aren't_ taken.
>
> That's the assumption I'm making, yes. The way basic block reordering
> works, is to try and make the most likely trace consist of
> fall-through paths.
>
> FWIW, the vast majority of condjumps have a REG_BR_PROB note so the
> part of mostly_true_jump that handles the jumps without a REG_BR_PROB
> note is itself a rare_destination :-) GCC is very careful about
> preserving the notes. Perhaps all that code should just be removed.
Yeah, just returning 0 if there's no note sounds good. Preapproved if
noone objects in 24 hours.
Thanks,
Richard
