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

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.

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

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!

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!

Ping? (Did I miss the PR?)