Page MenuHomePhabricator

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.Tue, Jul 21, 8:53 AM
rjf updated this revision to Diff 279983.EditedWed, Jul 22, 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.Wed, Jul 22, 5:43 PM

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

rjf commandeered this revision.Wed, Jul 22, 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.Thu, Jul 23, 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.EditedThu, Jul 23, 10:49 AM

Resolved merge conflicts.

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

Ping for review.

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

Add testcase for outlining cold block containing assert fail.

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

Add testcase for longjmp.

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

Squash commits into one.

rjf updated this revision to Diff 282136.Thu, Jul 30, 11:00 PM
This comment was removed by rjf.
rjf updated this revision to Diff 282223.Fri, Jul 31, 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.Fri, Jul 31, 9:15 AM

equals should be contains

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

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

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

LGTM, thanks for working on this.

This revision is now accepted and ready to land.Fri, Jul 31, 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.Tue, Aug 4, 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.