Page MenuHomePhabricator

[HotColdSplitting] Refine definition of unlikelyExecuted
ClosedPublic

Authored by vsk on Nov 7 2018, 5:13 PM.

Details

Summary

The splitting pass uses its 'unlikelyExecuted' predicate to statically
decide which blocks are cold.

  • Do not treat noreturn calls as if they are cold unless they are actually marked cold. This is motivated by functions like exit() and longjmp(), which are not beneficial to outline.
  • Do not treat inline asm as an outlining barrier. In practice asm("") is frequently used to inhibit basic block merging; enabling outlining in this case results in substantial memory savings.
  • Treat invokes of cold functions as cold.

As a drive-by, remove the 'exceptionHandlingFunctions' predicate, because it's
no longer needed. The pass can identify & outline blocks dominated by EH pads,
so there's no need to special-case __cxa_begin_catch etc.

Diff Detail

Event Timeline

vsk created this revision.Nov 7 2018, 5:13 PM
vsk edited the summary of this revision. (Show Details)

This is motivated by functions like exit() and longjmp(), which are not beneficial to outline.

exit() seems like a weird example: it executes at most once per process. What's the downside of outlining it?

vsk added a comment.Nov 7 2018, 5:39 PM

This is motivated by functions like exit() and longjmp(), which are not beneficial to outline.

exit() seems like a weird example: it executes at most once per process. What's the downside of outlining it?

It’s not clear that there’s an actual benefit to outlining it. A call to exit(0) could be expected to happen: outlining it to a cold section might force a page fault when it otherwise wouldn’t.

Thanks Vedant for handling this. Overall I agree with this change, but I believe it's good to break this patch down into three parts and get each of them reviewed separately :

  1. Mark @llvm.trap cold.
  2. Do not treat noreturn calls as cold unless they are actually marked cold.
  3. Do not treat inline asm as an outlining barrier.

Or

  1. Mark @llvm.trap cold.
  2. Do not treat noreturn calls as cold unless they are actually marked cold, and Do not treat inline asm as an outlining barrier.
vsk updated this revision to Diff 173378.Nov 9 2018, 10:45 AM
vsk edited the summary of this revision. (Show Details)
  • Split out the change to mark llvm.trap cold, per review feedback.
hiraditya accepted this revision.Nov 13 2018, 5:39 AM

Thanks for removing the exception handlers. I never wanted to hard code them in the first place.

It’s not clear that there’s an actual benefit to outlining it. A call to exit(0) could be expected to happen: outlining it to a cold section might force a page fault when it otherwise wouldn’t.

Agreed, also long jumps may incur page faults early in the program. Outlining them may have additional page faults. A program using co-routine features may see unexpected perf changes if we outline long jumps.

This revision is now accepted and ready to land.Nov 13 2018, 5:39 AM

Thanks for the update. Just one minor comment inlined.

llvm/lib/Transforms/IPO/HotColdSplitting.cpp
106–120

It seems that the case below will be ended up with warm. Don't we need to see this case as cold?

BB:
  ..... 
  call void @sink()
  ...... 
  tail call void @longjmp(%struct.__jmp_buf_tag* %1, i32 0)
  unreachable

declare void @sink() cold
declare void @longjmp(%struct.__jmp_buf_tag*, i32) noreturn nounwind
vsk updated this revision to Diff 174082.Nov 14 2018, 12:14 PM
vsk marked an inline comment as done.
vsk edited the summary of this revision. (Show Details)
  • Handle cold invokes, and address review feedback from @junbuml
llvm/lib/Transforms/IPO/HotColdSplitting.cpp
106–120

Thanks for pointing this out.

This revision was automatically updated to reflect the committed changes.