> 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.
> >> 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. 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
> 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 > >