Outline non returning functions unless a longjmp

[rjf]

Summary

__assert_fail, abort, exit etc. are cold.
TODO: outline throw

[hiraditya]

Will add test case soon.

[fhahn]

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.

[fhahn]

I wasn't reading that carefully enough, sorry! Still, please check for the longjmp intrinsic rather than just the name.

[hiraditya]

Added testcase

[hiraditya]

ping.

[hiraditya]

llvm/lib/Transforms/IPO/HotColdSplitting.cpp

122

TODO: if profile information is available, make use of that here.

[rjf]

Remove use of CallSite and update longjmp intrinsics (since Intrinsics::longjmp is removed, changed to using Intrinsics::eh_sjlj_longjmp).

[rjf]

Utilize profile information to mark longjmp blocks as unlikely executed if they are cold.

[rjf]

Commandeering this revision as discussed with @hiraditya .

llvm/lib/Transforms/IPO/HotColdSplitting.cpp

122

add use of profile information.

[rjf]

Seems like the patch isn't applying cleanly (which is causing the build failure). I'm looking into it.

[rjf]

Resolved merge conflicts.

[rjf]

Ping for review.

[rjf]

Add testcase for outlining cold block containing assert fail.

[rjf]

ping @hiraditya .

[rjf]

Add testcase for longjmp.

[rjf]

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

[rjf]

This comment was removed by rjf.

[rjf]

Squash commits into one.

[rjf]

This comment was removed by rjf.

[rjf]

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.

[rjf]

equals should be contains

[rjf]

Add testcases for c stdlib longjmp/setjmp functions in addition to builtin_longjmp and builtin_setjmp.

[hiraditya]

LGTM, thanks for working on this.

[CarolineConcatto]

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

[hiraditya]

I'm looking into it. Thanks for flagging.

[hiraditya]

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.

[CarolineConcatto]

So the build has as a target: cmake -DLLVM_TARGETS_TO_BUILD=AArch64

[rjf]

In D69257#2190753, @CarolineConcatto wrote:

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

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.

[vsk]

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

llvm/lib/Transforms/IPO/HotColdSplitting.cpp

if (auto CS = CallSite(&I))
       if (CS.hasFnAttr(Attribute::Cold) && !CS->getMetadata("nosanitize"))
         return true;
 
   // The block is cold if it has an unreachable terminator, unless it's
   // preceded by a call to a (possibly warm) noreturn call (e.g. longjmp).
   if (blockEndsInUnreachable(BB)) {
     if (auto *CI =
             dyn_cast_or_null<CallInst>(BB.getTerminator()->getPrevNode()))
-      if (CI->hasFnAttr(Attribute::NoReturn))
-        return false;
+      if (CI->hasFnAttr(Attribute::NoReturn)) {
+        if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(CI))
+          return II->getIntrinsicID() != Intrinsic::longjmp;
+        CallSite CS(CI);
+        return !CS.getCalledFunction()->getName().startswith("longjmp");
+      }
     return true;
   }
 
   return false;
 }
 
 /// Check whether it's safe to outline \p BB.
 static bool mayExtractBlock(const BasicBlock &BB) {