__assert_fail, abort, exit etc. are cold.
TODO: outline throw
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The longjmp intrinsic already should have the NoReturn property. Is the motivation here to handle calls of a function named longjmp, that is not the longjmp intrinsic? If that's the case, I do not think that is safe to do so. Unless it's the intrinsic, nothing ensures a function named longjmp actually corresponds to the library function.
I wasn't reading that carefully enough, sorry! Still, please check for the longjmp intrinsic rather than just the name.
llvm/lib/Transforms/IPO/HotColdSplitting.cpp | ||
---|---|---|
126 | TODO: if profile information is available, make use of that here. |
Remove use of CallSite and update longjmp intrinsics (since Intrinsics::longjmp is removed, changed to using Intrinsics::eh_sjlj_longjmp).
Utilize profile information to mark longjmp blocks as unlikely executed if they are cold.
Commandeering this revision as discussed with @hiraditya .
llvm/lib/Transforms/IPO/HotColdSplitting.cpp | ||
---|---|---|
126 | add use of profile information. |
Seems like the patch isn't applying cleanly (which is causing the build failure). I'm looking into it.
Update to include testcase with profile information, and check for "longjmp" suffix when detecting blocks containing a longjmp (since the intrinsic name is now "eh_sjlj_longjmp").
Check if function name equals longjmp when checking for blocks
including calls to longjmp; do not check for prefixes and suffixes
anymore, as it might cause potential problems in the future.
Also, eh_sjlj longjmp/setjmp calls will have an eh_sjlj intrinsic,
so the intrinsic check suffices.
Add testcases for c stdlib longjmp/setjmp functions in addition to builtin_longjmp and builtin_setjmp.
@hiraditya
We believe that this patch has caused the Flang buildbots to fail:
http://lab.llvm.org:8011/builders/flang-aarch64-ubuntu/builds/2437
http://lab.llvm.org:8011/builders/flang-aarch64-ubuntu-clang/builds/579
It would be nice if someone could take and action to revert of fix it ASAP.
Otherwise we cannot verify the other patches properly.
The error says:
LLVM ERROR: Trying to construct TargetPassConfig without a target machine. Scheduling a CodeGen pass without a target triple set? PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace. Stack dump:
Are we configuring the test correctly. Maybe the target triple in the test case needs to be removed.
I believe the fix in https://reviews.llvm.org/D85215 should be sufficient. This was in part caused by a problem in coldentrycount.ll in another patch: https://reviews.llvm.org/D69384. That should be fixed as well in D85215. Thanks for pointing this out.
FYI, this patch (https://reviews.llvm.org/D69257) cannot complete a stage2
build due to the change:
CI->getCalledFunction()->getName().contains("longjmp")
There are several concrete issues here:
- The callee may not be a function, so getCalledFunction can assert.
- The called value may not have a name, so getName can assert.
- There's no distinction made between "my_longjmp_test_helper" and the actual longjmp libcall.
At a higher level, there's a serious layering problem here. The
splitting pass makes policy decisions in a general way (e.g. based on
attributes or profile data). Special-casing certain names breaks the
layering. It subverts the work of library maintainers (who may now need
to opt-out of unexpected optimization behavior for any affected
functions) and can lead to inconsistent optimization behavior (as not
all llvm passes special-case ".*longjmp.*" in the same way).
The patch may need significant revision to address these issues.
But the immediate issue is that this crashes while compiling llvm's unit
tests in a stage2 build (due to the getName problem).
TODO: if profile information is available, make use of that here.