[opt-remark] Add support for emitting opt-remark-generator remarks when compiling with optimization.

In order to test this, I implemented a small source loc inference routine for
instructions without valid SILLocations. This is an optional nob that the
opt-remark writer can optionally enable on a per remark basis. The current
behaviors are just forward/backward scans in the same basic block. If we scan
forwards, if we find a valid SourceLoc, we just use ethat. If we are scanning
backwards, instead we grab the SourceRange and if it is valid use the end source
range of the given instruction. This seems to give a good result for retain
(forward scan) and release (backward scan).

The specific reason that I did that is that my test case for this are
retain/release operations. Often times these operations due to code-motion are
moved around (and rightly to prevent user confusion) given by optimizations auto
generated SIL locations. Since that is the test case I am using, to test this I
needed said engine.

Author: gottesmm

include/swift/SIL/OptimizationRemark.h

@@ -65,10 +65,29 @@ struct IndentDebug {
   unsigned width;
 };
 
+enum class SourceLocInferenceBehavior {
+  None,
+  ForwardScanOnly,
+  BackwardScanOnly,
+  ForwardThenBackward,
+  BackwardThenForward,
+};
+
+/// Infer the proper SourceLoc to use for the given SILInstruction.
+///
+/// This means that if we have a valid location for the instruction, we just
+/// return that. Otherwise, we have a runtime instruction that does not have a
+/// valid source location. In such a case, we infer the source location from the
+/// surrounding code. If we can not find any surrounding code, we return an
+/// invalid SourceLoc.
+SourceLoc inferOptRemarkSourceLoc(SILInstruction &i,
+                                  SourceLocInferenceBehavior inferBehavior);
+
 /// The base class for remarks.  This can be created by optimization passed to
 /// report successful and unsuccessful optimizations. CRTP is used to preserve
 /// the underlying type encoding the remark kind in the insertion operator.
-template <typename DerivedT> class Remark {
+template <typename DerivedT>
+class Remark {
   /// Arguments collected via the streaming interface.
   SmallVector<Argument, 4> args;
 
@@ -93,9 +112,10 @@ template <typename DerivedT> class Remark {
   unsigned indentDebugWidth = 0;
 
 protected:
-  Remark(StringRef identifier, SILInstruction &i)
+  Remark(StringRef identifier, SILInstruction &i,
+         SourceLocInferenceBehavior inferenceBehavior)
       : identifier((Twine("sil.") + identifier).str()),
-        location(i.getLoc().getSourceLoc()),
+        location(inferOptRemarkSourceLoc(i, inferenceBehavior)),
         function(i.getParent()->getParent()),
         demangledFunctionName(Demangle::demangleSymbolAsString(
             function->getName(),
@@ -133,11 +153,19 @@ template <typename DerivedT> class Remark {
 
 /// Remark to report a successful optimization.
 struct RemarkPassed : public Remark<RemarkPassed> {
-  RemarkPassed(StringRef id, SILInstruction &i) : Remark(id, i) {}
+  RemarkPassed(StringRef id, SILInstruction &i)
+      : Remark(id, i, SourceLocInferenceBehavior::None) {}
+  RemarkPassed(StringRef id, SILInstruction &i,
+               SourceLocInferenceBehavior inferenceBehavior)
+      : Remark(id, i, inferenceBehavior) {}
 };
 /// Remark to report a unsuccessful optimization.
 struct RemarkMissed : public Remark<RemarkMissed> {
-  RemarkMissed(StringRef id, SILInstruction &i) : Remark(id, i) {}
+  RemarkMissed(StringRef id, SILInstruction &i)
+      : Remark(id, i, SourceLocInferenceBehavior::None) {}
+  RemarkMissed(StringRef id, SILInstruction &i,
+               SourceLocInferenceBehavior inferenceBehavior)
+      : Remark(id, i, inferenceBehavior) {}
 };
 
 /// Used to emit the remarks.  Passes reporting remarks should create an

lib/SIL/Utils/OptimizationRemark.cpp

@@ -22,6 +22,7 @@
 #include "swift/Demangling/Demangler.h"
 #include "swift/SIL/SILRemarkStreamer.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace swift;
@@ -67,15 +68,17 @@ Argument::Argument(StringRef key, CanType ty) : key(key) {
   ty.print(stream);
 }
 
-template <typename DerivedT> std::string Remark<DerivedT>::getMsg() const {
+template <typename DerivedT>
+std::string Remark<DerivedT>::getMsg() const {
   std::string str;
   llvm::raw_string_ostream stream(str);
   for (const Argument &arg : args)
     stream << arg.val;
   return stream.str();
 }
 
-template <typename DerivedT> std::string Remark<DerivedT>::getDebugMsg() const {
+template <typename DerivedT>
+std::string Remark<DerivedT>::getDebugMsg() const {
   std::string str;
   llvm::raw_string_ostream stream(str);
 
@@ -100,6 +103,90 @@ Emitter::Emitter(StringRef passName, SILModule &m)
           m.getASTContext().LangOpts.OptimizationRemarkMissedPattern->match(
               passName)) {}
 
+/// The user has passed us an instruction that for some reason has a source loc
+/// that can not be used. Search down the current block for an instruction with
+/// a valid source loc and use that instead.
+static SourceLoc inferOptRemarkSearchForwards(SILInstruction &i) {
+  for (auto &inst :
+       llvm::make_range(std::next(i.getIterator()), i.getParent()->end())) {
+    auto newLoc = inst.getLoc().getSourceLoc();
+    if (newLoc.isValid())
+      return newLoc;
+  }
+
+  return SourceLoc();
+}
+
+/// The user has passed us an instruction that for some reason has a source loc
+/// that can not be used. Search up the current block for an instruction with
+/// a valid SILLocation and use the end SourceLoc of the SourceRange for the
+/// instruction.
+static SourceLoc inferOptRemarkSearchBackwards(SILInstruction &i) {
+  for (auto &inst : llvm::make_range(std::next(i.getReverseIterator()),
+                                     i.getParent()->rend())) {
+    auto loc = inst.getLoc();
+    if (!bool(loc))
+      continue;
+
+    auto range = loc.getSourceRange();
+    if (range.isValid())
+      return range.End;
+  }
+
+  return SourceLoc();
+}
+
+SourceLoc swift::OptRemark::inferOptRemarkSourceLoc(
+    SILInstruction &i, SourceLocInferenceBehavior inferBehavior) {
+  auto loc = i.getLoc().getSourceLoc();
+
+  // Do a quick check if we already have a valid loc. In such a case, just
+  // return. Otherwise, we try to infer using one of our heuristics below.
+  if (loc.isValid())
+    return loc;
+
+  // 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 loc;
+  case SourceLocInferenceBehavior::ForwardScanOnly: {
+    SourceLoc newLoc = inferOptRemarkSearchForwards(i);
+    if (newLoc.isValid())
+      return newLoc;
+    return loc;
+  }
+  case SourceLocInferenceBehavior::BackwardScanOnly: {
+    SourceLoc newLoc = inferOptRemarkSearchBackwards(i);
+    if (newLoc.isValid())
+      return newLoc;
+    return loc;
+  }
+  case SourceLocInferenceBehavior::ForwardThenBackward: {
+    SourceLoc newLoc = inferOptRemarkSearchForwards(i);
+    if (newLoc.isValid())
+      return newLoc;
+    newLoc = inferOptRemarkSearchBackwards(i);
+    if (newLoc.isValid())
+      return newLoc;
+    return loc;
+  }
+  case SourceLocInferenceBehavior::BackwardThenForward: {
+    SourceLoc newLoc = inferOptRemarkSearchBackwards(i);
+    if (newLoc.isValid())
+      return newLoc;
+    newLoc = inferOptRemarkSearchForwards(i);
+    if (newLoc.isValid())
+      return newLoc;
+    return loc;
+  }
+  }
+
+  llvm_unreachable("Covered switch isn't covered?!");
+}
+
 template <typename RemarkT, typename... ArgTypes>
 static void emitRemark(SILModule &module, const Remark<RemarkT> &remark,
                        Diag<ArgTypes...> id, bool diagEnabled) {

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -660,6 +660,9 @@ static void addLastChanceOptPassPipeline(SILPassPipelinePlan &P) {
 
   // Only has an effect if the -assume-single-thread option is specified.
   P.addAssumeSingleThreaded();
+
+  // Only has an effect if opt-remark is enabled.
+  P.addOptRemarkGenerator();
 }
 
 static void addSILDebugInfoGeneratorPipeline(SILPassPipelinePlan &P) {

lib/SILOptimizer/Transforms/OptRemarkGenerator.cpp

@@ -49,35 +49,43 @@ void OptRemarkGeneratorInstructionVisitor::visitStrongRetainInst(
     StrongRetainInst *sri) {
   ORE.emit([&]() {
     using namespace OptRemark;
-    return RemarkMissed("memory-management", *sri)
-           << "Unable to remove ARC operation";
+    // Retains begin a lifetime scope so we infer scan forward.
+    return RemarkMissed("memory-management", *sri,
+                        SourceLocInferenceBehavior::ForwardScanOnly)
+           << "Unable to remove retain";
   });
 }
 
 void OptRemarkGeneratorInstructionVisitor::visitStrongReleaseInst(
     StrongReleaseInst *sri) {
   ORE.emit([&]() {
     using namespace OptRemark;
-    return RemarkMissed("memory-management", *sri)
-           << "Unable to remove ARC operation";
+    // Releases end a lifetime scope so we infer scan backward.
+    return RemarkMissed("memory-management", *sri,
+                        SourceLocInferenceBehavior::BackwardScanOnly)
+           << "Unable to remove release";
   });
 }
 
 void OptRemarkGeneratorInstructionVisitor::visitRetainValueInst(
     RetainValueInst *rvi) {
   ORE.emit([&]() {
     using namespace OptRemark;
-    return RemarkMissed("memory-management", *rvi)
-           << "Unable to remove ARC operation";
+    // Retains begin a lifetime scope, so we infer scan forwards.
+    return RemarkMissed("memory-management", *rvi,
+                        SourceLocInferenceBehavior::ForwardScanOnly)
+           << "Unable to remove retain";
   });
 }
 
 void OptRemarkGeneratorInstructionVisitor::visitReleaseValueInst(
     ReleaseValueInst *rvi) {
   ORE.emit([&]() {
     using namespace OptRemark;
-    return RemarkMissed("memory-management", *rvi)
-           << "Unable to remove ARC operation";
+    // Releases end a lifetime scope so we infer scan backward.
+    return RemarkMissed("memory-management", *rvi,
+                        SourceLocInferenceBehavior::BackwardScanOnly)
+           << "Unable to remove release";
   });
 }
 
@@ -90,10 +98,22 @@ namespace {
 class OptRemarkGenerator : public SILFunctionTransform {
   ~OptRemarkGenerator() override {}
 
+  bool isOptRemarksEnabled() {
+    // TODO: Put this on LangOpts as a helper.
+    auto &langOpts = getFunction()->getASTContext().LangOpts;
+    return bool(langOpts.OptimizationRemarkMissedPattern) ||
+           bool(langOpts.OptimizationRemarkPassedPattern);
+  }
+
   /// The entry point to the transformation.
   void run() override {
-    OptRemarkGeneratorInstructionVisitor visitor(getFunction()->getModule());
-    for (auto &block : *getFunction()) {
+    if (!isOptRemarksEnabled())
+      return;
+
+    auto *fn = getFunction();
+    LLVM_DEBUG(llvm::dbgs() << "Visiting: " << fn->getName() << "\n");
+    OptRemarkGeneratorInstructionVisitor visitor(fn->getModule());
+    for (auto &block : *fn) {
       for (auto &inst : block) {
         visitor.visit(&inst);
       }

test/SILOptimizer/opt-remark-generator.sil

@@ -1,12 +1,12 @@
 // RUN: %target-sil-opt -sil-opt-remark-generator -sil-remarks-missed=sil-opt-remark-gen %s -o /dev/null 2>&1 | %FileCheck %s
 
-// CHECK: {{.*}}opt-remark-generator.sil:18:3: remark: Unable to remove ARC operation
+// CHECK: {{.*}}opt-remark-generator.sil:18:3: remark: Unable to remove retain
 // CHECK-NEXT: strong_retain
-// CHECK: {{.*}}opt-remark-generator.sil:19:3: remark: Unable to remove ARC operation
+// CHECK: {{.*}}opt-remark-generator.sil:19:3: remark: Unable to remove retain
 // CHECK-NEXT: retain_value
-// CHECK: {{.*}}opt-remark-generator.sil:20:3: remark: Unable to remove ARC operation
+// CHECK: {{.*}}opt-remark-generator.sil:20:3: remark: Unable to remove release
 // CHECK-NEXT: strong_release
-// CHECK: {{.*}}opt-remark-generator.sil:21:3: remark: Unable to remove ARC operation
+// CHECK: {{.*}}opt-remark-generator.sil:21:3: remark: Unable to remove release
 // CHECK-NEXT: release_value
 
 sil_stage canonical
@@ -21,4 +21,4 @@ bb0(%0 : $Builtin.NativeObject):
   release_value %0 : $Builtin.NativeObject
   %9999 = tuple()
   return %9999 : $()
-}
+}

test/SILOptimizer/opt-remark-generator.swift

@@ -0,0 +1,19 @@
+// RUN: %target-swiftc_driver -O -Rpass-missed=sil-opt-remark-gen -Xllvm -sil-disable-pass=FunctionSignatureOpts -emit-sil %s -o /dev/null -Xfrontend -verify
+
+public class Klass {}
+
+public var global = Klass()
+
+@inline(never)
+public func getGlobal() -> Klass {
+    return global // expected-remark @:5 {{Unable to remove retain}}
+}
+
+public func useGlobal() {
+    let x = getGlobal()
+    // Make sure that the retain msg is at the beginning of the print and the
+    // releases are the end of the print.
+    print(x) // expected-remark @:5 {{Unable to remove retain}}
+    // expected-remark @-1:12 {{Unable to remove release}}
+    // expected-remark @-2:12 {{Unable to remove release}}
+}