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

The reason why is that due to ARCCodeMotion, they could have moved enough that
the SourceLoc on them is incorrect. That is why you can see in the tests that I
had to update, I am moving the retain to the return statement from the body of
the function since the retain was now right before the return.

I also went in and cleaned up the logic here a little bit. What we do now is
that we have a notion of instructions that we /always/ infer SourceLocs for (rr)
and ones that if we have a valid non-inlined location we use (e.x.:
allocations).

This mucked a little bit with my ability to run SIL tests since the SIL tests
were relying on this not happening to rr so that we would emit remarks on the rr
instructions themselves. I added an option that disables the always infer
behavior for this test.

That being said at this point to me it seems like the SourceLoc inference stuff
is really tied to OptRemarkGenerator and I am going to see if I can move it to
there. But that is for a future commit on another day.

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'}}
 }