llvm dev - Jun 2015 - [LLVMdev] Assert in BlockFrequency pass

Hi, we got the following assert:

assert(!Working[0].isLoopHeader() && "entry block is a loop
header");

[in BlockFrequencyInfoImpl<BT>::tryToComputeMassInFunction(),
BlockFrequencyInfoImpl.h] on a Hexagon target - the entry block is a loop
header. Has someone seen this assert on other targets?

What would be a preferable way to fix it:
- extend BlockFrequency pass to handle this case, e.g. by inserting a
dummy entry block, or
- make this BlockFrequncy's assumption/requirement more clearly
articulated and avoid creating this situation in a preceding pass (in the
particular case, a machine-level loop rotation)?

Thanks,
Ivan

> On 2015-Jun-04, at 12:28, Ivan Baev <ibaev at codeaurora.org> wrote:
> 
> Hi, we got the following assert:
> 
> assert(!Working[0].isLoopHeader() && "entry block is a loop
header");
> 
> [in BlockFrequencyInfoImpl<BT>::tryToComputeMassInFunction(),
> BlockFrequencyInfoImpl.h] on a Hexagon target - the entry block is a loop
> header. Has someone seen this assert on other targets?
> 
> What would be a preferable way to fix it:
> - extend BlockFrequency pass to handle this case, e.g. by inserting a
> dummy entry block, or
> - make this BlockFrequncy's assumption/requirement more clearly
> articulated and avoid creating this situation in a preceding pass (in the
> particular case, a machine-level loop rotation)?
This is unlikely to be a bug in BFI.  BFI assumes that LoopInfo is
correct.  This has come up before when a pass promised (but failed) to
preserve LoopInfo.  I think it's well enough documented -- in that BFI
"requires" LoopInfo -- but if you have an idea of how to make it more
clear that BFI requires LoopInfo go ahead :).

Last time this came up it was suggested that we should have a better
way of testing/verifying that analyses have been correctly preserved,
and I think that would be the best way of improving the diagnostic
(since it would catch the problem right after machine-level loop
rotation fails to preserve it).

(Of course, it's possible there *is* a bug in BFI... let me know what
you find.)

> On 2015-Jun-04, at 12:45, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> 
>> On 2015-Jun-04, at 12:28, Ivan Baev <ibaev at codeaurora.org>
wrote:
>> 
>> Hi, we got the following assert:
>> 
>> assert(!Working[0].isLoopHeader() && "entry block is a
loop header");
>> 
>> [in BlockFrequencyInfoImpl<BT>::tryToComputeMassInFunction(),
>> BlockFrequencyInfoImpl.h] on a Hexagon target - the entry block is a
loop
>> header. Has someone seen this assert on other targets?
>> 
>> What would be a preferable way to fix it:
>> - extend BlockFrequency pass to handle this case, e.g. by inserting a
>> dummy entry block, or
>> - make this BlockFrequncy's assumption/requirement more clearly
>> articulated and avoid creating this situation in a preceding pass (in
the
>> particular case, a machine-level loop rotation)?
> 
> This is unlikely to be a bug in BFI.  BFI assumes that LoopInfo is
> correct.  This has come up before when a pass promised (but failed) to
> preserve LoopInfo.  I think it's well enough documented -- in that BFI
> "requires" LoopInfo -- but if you have an idea of how to make it
more
> clear that BFI requires LoopInfo go ahead :).
> 
> Last time this came up it was suggested that we should have a better
> way of testing/verifying that analyses have been correctly preserved,
> and I think that would be the best way of improving the diagnostic
> (since it would catch the problem right after machine-level loop
> rotation fails to preserve it).
> 
> (Of course, it's possible there *is* a bug in BFI... let me know what
> you find.)
> 
Reading the assertion, maybe there is a bug.  In LLVM IR, it's illegal
to have a backedge to the entry block.  When I rewrote BFI I assumed
the same was true of Machine IR.  Was I wrong?  Is it legal to have a
backedge to the entry block?  (Or, as I assumed in my previous mail,
did machine-level loop rotation fail to preserve LoopInfo?)

If it *is* legal to have a backedge to the entry block, then we should
update BFI to expect it.