[FIX] Avoid creating BFI when emitting remarks for dead functions

[weiwang]

Summary

Dead function has its body stripped away, and can cause various
analyses to panic. Also it does not make sense to apply analyses on
such function.

[weiwang]

To reproduce:

1. create test file main.cpp

int bar() {
  return 2;
}

int foo() {
  return 1;
}

int main() {
  foo();
  return 0;
}

2. compile with clang: -fuse-ld=lld -O3 -flto=full -Wl,--opt-remarks-with-hotness -o test main.cpp

If clang has assertion enabled, it would hit the following assertion at ilist_iterator.h:138:

Assertion `!NodePtr->isKnownSentinel()' failed

otherwise, the behavior is undefined.

It is triggered at the point where ORE is created to emit dead functions LTO.cpp:801.

[wenlei]

I'm guessing these are not the only passes that will choke with empty/dead functions. If this is because ORE is being used for a special case, then we might want to fix the special case on the user side.

If we want passes to absorb such empty functions, then there needs to be consistency for all passes. I don't think passes need to absorb such cases though, as this is really a "limbo state" of functions when being removed (a normal empty function should still have entry block and ret instruction).

[MaskRay]

Test?

llvm/include/llvm/Support/GenericDomTreeConstruction.h

534

llvm-project currently requires C++14. C++17 extensions can't be used. (As an exception, if init-statement is allowed)

[fhahn]

llvm/lib/Analysis/BranchProbabilityInfo.cpp

1071

Are empty functions valid IR? Don't they have to have at least a single BB. And each BB needs at least a terminator?

[weiwang]

In D84715#2177627, @wenlei wrote:

I'm guessing these are not the only passes that will choke with empty/dead functions. If this is because ORE is being used for a special case, then we might want to fix the special case on the user side.

If we want passes to absorb such empty functions, then there needs to be consistency for all passes. I don't think passes need to absorb such cases though, as this is really a "limbo state" of functions when being removed (a normal empty function should still have entry block and ret instruction).

Agree. The proposed checks are only placed along the particular call path that leads to the failure. If we are going cover all the analyses/passes, the overall effort could be substantial.

If the big assumption is that there shouldn't be an empty body function to deal with, we can probably change how ORE is created at the beginning. Instead of creating a heavy-weight version of ORE (with all dependent analyses invoked), using the light-weight version OptimizationRemarkEmitter ORE(F, nullptr) at LTO.cpp:801.

llvm/lib/Analysis/BranchProbabilityInfo.cpp

1071

This is a special case where function is marked dead and its body is completed strip away. AFAIK, it only happens before LTO backend, so analysis/pass in pass pipeline won't see such function.

[weiwang]

add test case

[hoyFB]

In D84715#2179099, @weiwang wrote:

In D84715#2177627, @wenlei wrote:

I'm guessing these are not the only passes that will choke with empty/dead functions. If this is because ORE is being used for a special case, then we might want to fix the special case on the user side.

If we want passes to absorb such empty functions, then there needs to be consistency for all passes. I don't think passes need to absorb such cases though, as this is really a "limbo state" of functions when being removed (a normal empty function should still have entry block and ret instruction).

Agree. The proposed checks are only placed along the particular call path that leads to the failure. If we are going cover all the analyses/passes, the overall effort could be substantial.

If the big assumption is that there shouldn't be an empty body function to deal with, we can probably change how ORE is created at the beginning. Instead of creating a heavy-weight version of ORE (with all dependent analyses invoked), using the light-weight version OptimizationRemarkEmitter ORE(F, nullptr) at LTO.cpp:801.

Can we bail out for dead/empty functions right there before launching ORE on them?

[wenlei]

In D84715#2186299, @hoyFB wrote:

In D84715#2179099, @weiwang wrote:

In D84715#2177627, @wenlei wrote:

I'm guessing these are not the only passes that will choke with empty/dead functions. If this is because ORE is being used for a special case, then we might want to fix the special case on the user side.

If we want passes to absorb such empty functions, then there needs to be consistency for all passes. I don't think passes need to absorb such cases though, as this is really a "limbo state" of functions when being removed (a normal empty function should still have entry block and ret instruction).

Agree. The proposed checks are only placed along the particular call path that leads to the failure. If we are going cover all the analyses/passes, the overall effort could be substantial.

If the big assumption is that there shouldn't be an empty body function to deal with, we can probably change how ORE is created at the beginning. Instead of creating a heavy-weight version of ORE (with all dependent analyses invoked), using the light-weight version OptimizationRemarkEmitter ORE(F, nullptr) at LTO.cpp:801.

Can we bail out for dead/empty functions right there before launching ORE on them?

I think the error path involved here was specifically for emitting remarks about dead functions. So bailout for that case effectively means removing ORE there. With that, something like OptimizationRemarkEmitter ORE(F, nullptr) for that path seem to make sense.

[weiwang]

Instead of adding checks for all passes/analyses, use light-weight ORE to emit remarks for dead functions.

[MaskRay]

LG. Would be wise waiting for a second opinion

llvm/test/LTO/Resolution/X86/dead-strip-fulllto.ll

4 ↗

(On Diff #286156)

Add a comment about --pass-remarks-with-hotness

[wenlei]

LTGM too. Thanks for the fix!

[xazax.hun]

Thanks! Sorry for not noticing this earlier. The fix looks good to me and indeed the original intention was to report dead functions with ORE.

[weiwang]

add comment on test case.