This is an archive of the discontinued LLVM Phabricator instance.

Outline non returning functions unless a longjmp
ClosedPublic

Authored by rjf on Oct 21 2019, 6:38 AM.

Details

Summary

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

Diff Detail

Event Timeline

hiraditya created this revision.Oct 21 2019, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2019, 6:38 AM

Will add test case soon.

fhahn added a comment.Oct 21 2019, 9:36 AM

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 added a comment.Oct 21 2019, 9:45 AM

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

hiraditya updated this revision to Diff 225957.Oct 21 2019, 2:33 PM
hiraditya updated this revision to Diff 225958.
hiraditya updated this revision to Diff 226325.Oct 24 2019, 2:15 PM

Added testcase

hiraditya marked an inline comment as done.Mar 31 2020, 6:15 AM
hiraditya added inline comments.
llvm/lib/Transforms/IPO/HotColdSplitting.cpp
126

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

rjf added a subscriber: rjf.Jul 21 2020, 8:53 AM
rjf updated this revision to Diff 279983.EditedJul 22 2020, 5:36 PM

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

rjf updated this revision to Diff 279985.Jul 22 2020, 5:43 PM

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

rjf commandeered this revision.Jul 22 2020, 9:39 PM
rjf added a reviewer: hiraditya.

Commandeering this revision as discussed with @hiraditya .

llvm/lib/Transforms/IPO/HotColdSplitting.cpp
126

add use of profile information.

rjf marked an inline comment as done.Jul 23 2020, 10:18 AM

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

rjf updated this revision to Diff 280199.EditedJul 23 2020, 10:49 AM

Resolved merge conflicts.

rjf added a comment.Jul 23 2020, 11:24 AM

Ping for review.

rcorcs added a subscriber: rcorcs.Jul 23 2020, 2:50 PM
rjf updated this revision to Diff 281454.Jul 28 2020, 8:02 PM

Add testcase for outlining cold block containing assert fail.

rjf updated this revision to Diff 281804.Jul 29 2020, 11:03 PM

Add testcase for longjmp.

rjf updated this revision to Diff 282131.Jul 30 2020, 10:39 PM

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 updated this revision to Diff 282133.Jul 30 2020, 10:46 PM
This comment was removed by rjf.
rjf updated this revision to Diff 282135.Jul 30 2020, 10:55 PM

Squash commits into one.

rjf updated this revision to Diff 282136.Jul 30 2020, 11:00 PM
This comment was removed by rjf.
rjf updated this revision to Diff 282223.Jul 31 2020, 7:54 AM

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 updated this revision to Diff 282243.Jul 31 2020, 9:15 AM

equals should be contains

rjf updated this revision to Diff 282248.Jul 31 2020, 9:29 AM

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

hiraditya accepted this revision.Jul 31 2020, 11:08 AM

LGTM, thanks for working on this.

This revision is now accepted and ready to land.Jul 31 2020, 11:08 AM

@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'm looking into it. Thanks for flagging.

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.

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

rjf added a comment.Aug 4 2020, 9:00 AM

@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 added a subscriber: vsk.Oct 5 2020, 2:13 PM

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