Page MenuHomePhabricator

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

Authored by weiwang on Jul 27 2020, 4:52 PM.

Details

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.

Diff Detail

Event Timeline

weiwang created this revision.Jul 27 2020, 4:52 PM
weiwang edited the summary of this revision. (Show Details)Jul 27 2020, 4:53 PM
weiwang edited the summary of this revision. (Show Details)Jul 27 2020, 5:10 PM
weiwang added a comment.EditedJul 27 2020, 5:15 PM

To reproduce:

  1. create test file main.cpp
int bar() {
  return 2;
}

int foo() {
  return 1;
}

int main() {
  foo();
  return 0;
}
  1. 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.

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

Test?

llvm/include/llvm/Support/GenericDomTreeConstruction.h
534 ↗(On Diff #281083)

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

fhahn added a subscriber: fhahn.Jul 28 2020, 2:00 AM
fhahn added inline comments.
llvm/lib/Analysis/BranchProbabilityInfo.cpp
1071 ↗(On Diff #281083)

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

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 ↗(On Diff #281083)

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 updated this revision to Diff 282085.Jul 30 2020, 5:23 PM

add test case

hoyFB added a subscriber: hoyFB.Jul 30 2020, 5:26 PM

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'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 updated this revision to Diff 286156.Aug 17 2020, 3:14 PM

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

weiwang retitled this revision from [FIX] Add check for empty body function to [FIX] Avoid creating BFI when emitting remarks for dead functions.Aug 17 2020, 3:15 PM
weiwang edited the summary of this revision. (Show Details)
weiwang edited the summary of this revision. (Show Details)
MaskRay accepted this revision.Aug 17 2020, 4:21 PM

LG. Would be wise waiting for a second opinion

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

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

This revision is now accepted and ready to land.Aug 17 2020, 4:21 PM
wenlei accepted this revision.Aug 17 2020, 7:01 PM

LTGM too. Thanks for the fix!

xazax.hun accepted this revision.Aug 17 2020, 9:36 PM

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.

hoy accepted this revision.Aug 18 2020, 8:37 PM
weiwang updated this revision to Diff 286471.Aug 18 2020, 8:53 PM

add comment on test case.

weiwang marked an inline comment as done.Aug 18 2020, 8:53 PM
This revision was landed with ongoing or failed builds.Aug 25 2020, 11:12 AM
This revision was automatically updated to reflect the committed changes.