Merge pull request swiftlang#33820 from gottesmm/pr-2022a20c73c213c8d5041a6de01d0874cd466764

[opt-remark] Infer the source loc retain, release even if they have non-inlined locs.

Author: gottesmm

include/swift/SIL/OptimizationRemark.h

@@ -137,14 +137,26 @@ struct IndentDebug {
   unsigned width;
 };
 
-enum class SourceLocInferenceBehavior {
-  None,
-  ForwardScanOnly,
-  BackwardScanOnly,
-  ForwardThenBackward,
-  BackwardThenForward,
+enum class SourceLocInferenceBehavior : unsigned {
+  None = 0,
+  ForwardScan = 0x1,
+  BackwardScan = 0x2,
+  ForwardScan2nd = 0x4,
+  AlwaysInfer = 0x8,
+
+  ForwardThenBackwards = ForwardScan | BackwardScan,
+  BackwardsThenForwards = BackwardScan | ForwardScan2nd,
+  ForwardScanAlwaysInfer = ForwardScan | AlwaysInfer,
+  BackwardScanAlwaysInfer = BackwardScan | AlwaysInfer,
 };
 
+inline SourceLocInferenceBehavior operator&(SourceLocInferenceBehavior lhs,
+                                            SourceLocInferenceBehavior rhs) {
+  auto lhsVal = std::underlying_type<SourceLocInferenceBehavior>::type(lhs);
+  auto rhsVal = std::underlying_type<SourceLocInferenceBehavior>::type(rhs);
+  return SourceLocInferenceBehavior(lhsVal & rhsVal);
+}
+
 /// Infer the proper SourceLoc to use for the given SILInstruction.
 ///
 /// This means that if we have a valid location for the instruction, we just

lib/SIL/Utils/OptimizationRemark.cpp

@@ -182,59 +182,48 @@ static SourceLoc inferOptRemarkSearchBackwards(SILInstruction &i) {
   return SourceLoc();
 }
 
+static llvm::cl::opt<bool> IgnoreAlwaysInferForTesting(
+    "sil-opt-remark-ignore-always-infer", llvm::cl::Hidden,
+    llvm::cl::init(false),
+    llvm::cl::desc(
+        "Disables always infer source loc behavior for testing purposes"));
+
+// Attempt to infer a SourceLoc for \p i using heuristics specified by \p
+// inferBehavior.
+//
+// NOTE: We distinguish in between situations where we always must infer
+// (retain, release) and other situations where we are ok with original source
+// locs if we are not inlined (alloc_ref, alloc_stack).
 SourceLoc swift::OptRemark::inferOptRemarkSourceLoc(
     SILInstruction &i, SourceLocInferenceBehavior inferBehavior) {
-  // Do a quick check if we already have a valid loc and it isnt an inline
-  // loc. In such a case, just return. Otherwise, we try to infer using one of
-  // our heuristics below.
+  // If we are only supposed to infer in inline contexts, see if we have a valid
+  // loc and if that loc is an inlined call site.
   auto loc = i.getLoc();
-  if (loc.getSourceLoc().isValid()) {
-    // Before we do anything, if we do not have an inlined call site, just
-    // return our loc.
-    if (!i.getDebugScope() || !i.getDebugScope()->InlinedCallSite)
-      return loc.getSourceLoc();
-  }
+  if (loc.getSourceLoc().isValid() &&
+      !(bool(inferBehavior & SourceLocInferenceBehavior::AlwaysInfer) &&
+        !IgnoreAlwaysInferForTesting) &&
+      !(i.getDebugScope() && i.getDebugScope()->InlinedCallSite))
+    return loc.getSourceLoc();
 
-  // Otherwise, try to handle the individual behavior cases, returning loc at
-  // the end of each case (its invalid, so it will get ignored). If loc is not
-  // returned, we hit an assert at the end to make it easy to identify a case
-  // was missed.
-  switch (inferBehavior) {
-  case SourceLocInferenceBehavior::None:
-    return SourceLoc();
-  case SourceLocInferenceBehavior::ForwardScanOnly: {
+  if (bool(inferBehavior & SourceLocInferenceBehavior::ForwardScan)) {
     SourceLoc newLoc = inferOptRemarkSearchForwards(i);
     if (newLoc.isValid())
       return newLoc;
-    return SourceLoc();
   }
-  case SourceLocInferenceBehavior::BackwardScanOnly: {
+
+  if (bool(inferBehavior & SourceLocInferenceBehavior::BackwardScan)) {
     SourceLoc newLoc = inferOptRemarkSearchBackwards(i);
     if (newLoc.isValid())
       return newLoc;
-    return SourceLoc();
   }
-  case SourceLocInferenceBehavior::ForwardThenBackward: {
+
+  if (bool(inferBehavior & SourceLocInferenceBehavior::ForwardScan2nd)) {
     SourceLoc newLoc = inferOptRemarkSearchForwards(i);
     if (newLoc.isValid())
       return newLoc;
-    newLoc = inferOptRemarkSearchBackwards(i);
-    if (newLoc.isValid())
-      return newLoc;
-    return SourceLoc();
-  }
-  case SourceLocInferenceBehavior::BackwardThenForward: {
-    SourceLoc newLoc = inferOptRemarkSearchBackwards(i);
-    if (newLoc.isValid())
-      return newLoc;
-    newLoc = inferOptRemarkSearchForwards(i);
-    if (newLoc.isValid())
-      return newLoc;
-    return SourceLoc();
-  }
   }
 
-  llvm_unreachable("Covered switch isn't covered?!");
+  return SourceLoc();
 }
 
 template <typename RemarkT, typename... ArgTypes>

lib/SILOptimizer/Transforms/OptRemarkGenerator.cpp

@@ -425,10 +425,11 @@ void OptRemarkGeneratorInstructionVisitor::visitStrongRetainInst(
     (void)foundArgs;
 
     // Retains begin a lifetime scope so we infer scan forward.
-    auto remark = RemarkMissed("memory", *sri,
-                               SourceLocInferenceBehavior::ForwardScanOnly)
-                  << "retain of type '"
-                  << NV("ValueType", sri->getOperand()->getType()) << "'";
+    auto remark =
+        RemarkMissed("memory", *sri,
+                     SourceLocInferenceBehavior::ForwardScanAlwaysInfer)
+        << "retain of type '" << NV("ValueType", sri->getOperand()->getType())
+        << "'";
     for (auto arg : inferredArgs) {
       remark << arg;
     }
@@ -446,10 +447,11 @@ void OptRemarkGeneratorInstructionVisitor::visitStrongReleaseInst(
                                                sri->getOperand(), inferredArgs);
     (void)foundArgs;
 
-    auto remark = RemarkMissed("memory", *sri,
-                               SourceLocInferenceBehavior::BackwardScanOnly)
-                  << "release of type '"
-                  << NV("ValueType", sri->getOperand()->getType()) << "'";
+    auto remark =
+        RemarkMissed("memory", *sri,
+                     SourceLocInferenceBehavior::BackwardScanAlwaysInfer)
+        << "release of type '" << NV("ValueType", sri->getOperand()->getType())
+        << "'";
     for (auto arg : inferredArgs) {
       remark << arg;
     }
@@ -466,10 +468,11 @@ void OptRemarkGeneratorInstructionVisitor::visitRetainValueInst(
                                                rvi->getOperand(), inferredArgs);
     (void)foundArgs;
     // Retains begin a lifetime scope, so we infer scan forwards.
-    auto remark = RemarkMissed("memory", *rvi,
-                               SourceLocInferenceBehavior::ForwardScanOnly)
-                  << "retain of type '"
-                  << NV("ValueType", rvi->getOperand()->getType()) << "'";
+    auto remark =
+        RemarkMissed("memory", *rvi,
+                     SourceLocInferenceBehavior::ForwardScanAlwaysInfer)
+        << "retain of type '" << NV("ValueType", rvi->getOperand()->getType())
+        << "'";
     for (auto arg : inferredArgs) {
       remark << arg;
     }
@@ -487,10 +490,11 @@ void OptRemarkGeneratorInstructionVisitor::visitReleaseValueInst(
     (void)foundArgs;
 
     // Releases end a lifetime scope so we infer scan backward.
-    auto remark = RemarkMissed("memory", *rvi,
-                               SourceLocInferenceBehavior::BackwardScanOnly)
-                  << "release of type '"
-                  << NV("ValueType", rvi->getOperand()->getType()) << "'";
+    auto remark =
+        RemarkMissed("memory", *rvi,
+                     SourceLocInferenceBehavior::BackwardScanAlwaysInfer)
+        << "release of type '" << NV("ValueType", rvi->getOperand()->getType())
+        << "'";
     for (auto arg : inferredArgs) {
       remark << arg;
     }
@@ -508,8 +512,7 @@ void OptRemarkGeneratorInstructionVisitor::visitAllocRefInst(
           valueToDeclInferrer.infer(ArgumentKeyKind::Note, ari, inferredArgs);
       (void)foundArgs;
       auto resultRemark =
-          RemarkPassed("memory", *ari,
-                       SourceLocInferenceBehavior::ForwardScanOnly)
+          RemarkPassed("memory", *ari, SourceLocInferenceBehavior::ForwardScan)
           << "stack allocated ref of type '" << NV("ValueType", ari->getType())
           << "'";
       for (auto &arg : inferredArgs)
@@ -526,8 +529,7 @@ void OptRemarkGeneratorInstructionVisitor::visitAllocRefInst(
     (void)foundArgs;
 
     auto resultRemark =
-        RemarkMissed("memory", *ari,
-                     SourceLocInferenceBehavior::ForwardScanOnly)
+        RemarkMissed("memory", *ari, SourceLocInferenceBehavior::ForwardScan)
         << "heap allocated ref of type '" << NV("ValueType", ari->getType())
         << "'";
     for (auto &arg : inferredArgs)
@@ -546,8 +548,7 @@ void OptRemarkGeneratorInstructionVisitor::visitAllocBoxInst(
     (void)foundArgs;
 
     auto resultRemark =
-        RemarkMissed("memory", *abi,
-                     SourceLocInferenceBehavior::ForwardScanOnly)
+        RemarkMissed("memory", *abi, SourceLocInferenceBehavior::ForwardScan)
         << "heap allocated box of type '" << NV("ValueType", abi->getType())
         << "'";
     for (auto &arg : inferredArgs)

test/SILOptimizer/opt-remark-generator.sil

@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -optremarkgen-declless-debugvalue-use-sildebugvar-info -sil-opt-remark-generator -sil-remarks-missed=sil-opt-remark-gen -verify %s -o /dev/null
+// RUN: %target-sil-opt -sil-opt-remark-ignore-always-infer -optremarkgen-declless-debugvalue-use-sildebugvar-info -sil-opt-remark-generator -sil-remarks-missed=sil-opt-remark-gen -verify %s -o /dev/null
 
 sil_stage canonical
 

test/SILOptimizer/opt-remark-generator.swift

@@ -90,8 +90,10 @@ func printKlassPairLHS(x : KlassPair) {
                  // expected-remark @-3:16 {{release of type}}
 }
 
+// We put the retain on the return here since it is part of the result
+// convention.
 func returnKlassPairLHS(x: KlassPair) -> Klass {
-    return x.lhs // expected-remark @:14 {{retain of type 'Klass'}}
+    return x.lhs // expected-remark @:5 {{retain of type 'Klass'}}
                  // expected-note @-2:25 {{of 'x.lhs'}}
 }
 
@@ -123,7 +125,7 @@ func printKlassTupleLHS(x : (Klass, Klass)) {
 }
 
 func returnKlassTupleLHS(x: (Klass, Klass)) -> Klass {
-    return x.0 // expected-remark @:12 {{retain of type 'Klass'}}
+    return x.0 // expected-remark @:5 {{retain of type 'Klass'}}
                // expected-note @-2:26 {{of 'x'}}
 }