[LLVMdev] Assert in BlockFrequency pass

[Duncan P. N. Exon Smith]

> On 2015 Jun 5, at 18:08, Ivan Baev <ibaev at codeaurora.org> wrote:
> 
>> 
>>> 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.
> 
> 
> For LLVM IR it is not legal to have a backedge to the entry block - this
> is from Verifier.cpp
>    // Check the entry node
>    const BasicBlock *Entry = &F.getEntryBlock();
>    Assert1(pred_empty(Entry),
>            "Entry block to function must not have predecessors!", Entry);
> 
> Not sure about such requirement for Machine IR.

I can't find any mention of my assumption in the machine IR
documentation; it looks like I was just wrong :/.

This shouldn't be hard to fix -- I just need to massage some logic
here and there -- but I'll need your help coming up with a testcase.
Please file a PR with the testcase attached and CC me, and I'll fix it
as soon as I can!

> 
> For the test case with the assert, MBB below is the entry BB.
> [Loop rotate transformation in HexagonCFGOptimizer.cpp]
> // We are looking for the commonly occuring patterns.
> // MBB ---------
> //  |           |
> // (Optional    |
> //  Preheader)  |
> //  |           |
> // MBB1 <---    |
> //  |       |   |
> // ...(Loop)|   |
> //  |       |   |
> // MBB2 ----    |
> //  |           |
> // MBBExit <----
> //
> // This pattern will be transformed into the following loop.
> // i.e. the predecessor will be included in the loop.
> //   ,<---------
> //  |           |
> // MBB ->------- -
> //  |           | |
> // (Optional    | |
> //  Preheader)  | |
> //  |           | |
> // MBB1         | |
> //  |           | |
> // ...(Loop)    | |
> //  |           | |
> // MBB2 ->------  |
> //  |             |
> // MBBExit <------
> 
> HexagonCFGOptimizer pass does not promise to preserve LoopInfo.
> 
> Ivan
> 
>