Merge pull request #23375 from bob-wilson/llvm-r355981

Stop using the _branchHint function

Author: bob-wilson

docs/StandardLibraryProgrammersManual.md

@@ -53,15 +53,15 @@ Optionals can be unwrapped with `!`, which triggers a trap on nil. Alternatively
 
 ### Builtins
 
-#### `_fastPath` and `_slowPath` (also, `_branchHint`)
+#### `_fastPath` and `_slowPath`
 
 `_fastPath` returns its argument, wrapped in a Builtin.expect. This informs the optimizer that the vast majority of the time, the branch will be taken (i.e. the then branch is “hot”). The SIL optimizer may alter heuristics for anything dominated by the then branch. But the real performance impact comes from the fact that the SIL optimizer will consider anything dominated by the else branch to be infrequently executed (i.e. “cold”). This means that transformations that may increase code size have very conservative heuristics to keep the rarely executed code small.
 
 The probabilities are passed through to LLVM as branch weight metadata, to leverage LLVM’s use of GCC style builtin_expect knowledge (e.g. for code layout and low-level inlining).
 
 `_fastPath` probabilities are compounding, see the example below. For this reason, it can actually degrade performance in non-intuitive ways as it marks all other code (including subsequent `_fastPath`s) as being cold. Consider `_fastPath` as basically spraying the rest of the code with a Mr. Freeze-style ice gun.
 
-`_slowPath` is the same as `_fastPath`, just with the branches swapped. Both are just wrappers around `_branchHint`, which is otherwise never called directly.
+`_slowPath` is the same as `_fastPath`, just with the branches swapped.
 
 *Example:*
 
@@ -84,8 +84,6 @@ if _fastPath(...) {
 return
 ```
 
-*NOTE: these are due for a rename and possibly a redesign. They conflate multiple notions that don’t match the average standard library programmer’s intuition.*
-
 
 #### `_onFastPath`
 

lib/SILOptimizer/Analysis/ColdBlockInfo.cpp

@@ -74,30 +74,16 @@ ColdBlockInfo::BranchHint ColdBlockInfo::getBranchHint(SILValue Cond,
     return Hint;
   }
 
-  // Handle the @semantic function used for branch hints. The generic
-  // fast/slowPath calls are frequently only inlined one level down to
-  // _branchHint before inlining the call sites that they guard.
+  // Handle the @semantic functions used for branch hints.
   auto AI = dyn_cast<ApplyInst>(Cond);
   if (!AI)
     return BranchHint::None;
 
   if (auto *F = AI->getReferencedFunction()) {
     if (F->hasSemanticsAttrs()) {
-      if (F->hasSemanticsAttr("branchhint")) {
-        // A "branchint" model takes a Bool expected value as the second
-        // argument.
-        if (auto *SI = dyn_cast<StructInst>(AI->getArgument(1))) {
-          assert(SI->getElements().size() == 1 && "Need Bool.value");
-          if (auto *Literal =
-              dyn_cast<IntegerLiteralInst>(SI->getElements()[0])) {
-            return (Literal->getValue() == 0)
-              ? BranchHint::LikelyFalse : BranchHint::LikelyTrue;
-          }
-        }
-      }
       // fastpath/slowpath attrs are untested because the inliner luckily
       // inlines them before the downstream calls.
-      else if (F->hasSemanticsAttr("slowpath"))
+      if (F->hasSemanticsAttr("slowpath"))
         return BranchHint::LikelyFalse;
       else if (F->hasSemanticsAttr("fastpath"))
         return BranchHint::LikelyTrue;

stdlib/public/core/Assert.swift

@@ -45,7 +45,7 @@ public func assert(
 ) {
   // Only assert in debug mode.
   if _isDebugAssertConfiguration() {
-    if !_branchHint(condition(), expected: true) {
+    if !_fastPath(condition()) {
       _assertionFailure("Assertion failed", message(), file: file, line: line,
         flags: _fatalErrorFlags())
     }
@@ -87,7 +87,7 @@ public func precondition(
 ) {
   // Only check in debug and release mode. In release mode just trap.
   if _isDebugAssertConfiguration() {
-    if !_branchHint(condition(), expected: true) {
+    if !_fastPath(condition()) {
       _assertionFailure("Precondition failed", message(), file: file, line: line,
         flags: _fatalErrorFlags())
     }
@@ -206,7 +206,7 @@ internal func _precondition(
 ) {
   // Only check in debug and release mode. In release mode just trap.
   if _isDebugAssertConfiguration() {
-    if !_branchHint(condition(), expected: true) {
+    if !_fastPath(condition()) {
       _fatalErrorMessage("Fatal error", message, file: file, line: line,
         flags: _fatalErrorFlags())
     }
@@ -235,7 +235,7 @@ public func _overflowChecked<T>(
 ) -> T {
   let (result, error) = args
   if _isDebugAssertConfiguration() {
-    if _branchHint(error, expected: false) {
+    if _slowPath(error) {
       _fatalErrorMessage("Fatal error", "Overflow/underflow", 
         file: file, line: line, flags: _fatalErrorFlags())
     }
@@ -260,7 +260,7 @@ internal func _debugPrecondition(
 ) {
   // Only check in debug mode.
   if _slowPath(_isDebugAssertConfiguration()) {
-    if !_branchHint(condition(), expected: true) {
+    if !_fastPath(condition()) {
       _fatalErrorMessage("Fatal error", message, file: file, line: line,
         flags: _fatalErrorFlags())
     }
@@ -290,7 +290,7 @@ internal func _internalInvariant(
   file: StaticString = #file, line: UInt = #line
 ) {
 #if INTERNAL_CHECKS_ENABLED
-  if !_branchHint(condition(), expected: true) {
+  if !_fastPath(condition()) {
     _fatalErrorMessage("Fatal error", message, file: file, line: line,
       flags: _fatalErrorFlags())
   }

stdlib/public/core/Builtin.swift

@@ -273,24 +273,18 @@ internal func _minAllocationAlignment() -> Int {
 // semantics of these function calls. This won't be necessary with
 // mandatory generic inlining.
 
-@usableFromInline @_transparent
-@_semantics("branchhint")
-internal func _branchHint(_ actual: Bool, expected: Bool) -> Bool {
-  return Bool(Builtin.int_expect_Int1(actual._value, expected._value))
-}
-
 /// Optimizer hint that `x` is expected to be `true`.
 @_transparent
 @_semantics("fastpath")
 public func _fastPath(_ x: Bool) -> Bool {
-  return _branchHint(x, expected: true)
+  return Bool(Builtin.int_expect_Int1(x._value, true._value))
 }
 
 /// Optimizer hint that `x` is expected to be `false`.
 @_transparent
 @_semantics("slowpath")
 public func _slowPath(_ x: Bool) -> Bool {
-  return _branchHint(x, expected: false)
+  return Bool(Builtin.int_expect_Int1(x._value, false._value))
 }
 
 /// Optimizer hint that the code where this function is called is on the fast
@@ -307,6 +301,17 @@ func _uncheckedUnsafeAssume(_ condition: Bool) {
   _ = Builtin.assume_Int1(condition._value)
 }
 
+// This function is no longer used but must be kept for ABI compatibility
+// because references to it may have been inlined.
+@usableFromInline
+internal func _branchHint(_ actual: Bool, expected: Bool) -> Bool {
+  // The LLVM intrinsic underlying int_expect_Int1 now requires an immediate
+  // argument for the expected value so we cannot call it here. This should
+  // never be called in cases where performance matters, so just return the
+  // value without any branch hint.
+  return actual
+}
+
 //===--- Runtime shim wrappers --------------------------------------------===//
 
 /// Returns `true` iff the class indicated by `theClass` uses native

test/SILOptimizer/performance_inliner.sil

@@ -752,7 +752,7 @@ bb0:
 }
 
 // Generic call to "branchHint" for use in specialized @slowPath
-sil public_external [transparent] [_semantics "branchhint"] @_TFs11_branchHintUs7Boolean__FTQ_Sb_Sb : $@convention(thin)(Bool, Bool) -> Bool {
+sil public_external [transparent] @_TFs11_branchHintUs7Boolean__FTQ_Sb_Sb : $@convention(thin)(Bool, Bool) -> Bool {
 bb0(%0 : $Bool, %1 : $Bool):
   return %0 : $Bool
 }