Merge pull request #32986 from gottesmm/pr-967e8caef81ac33bc24c69828271796b964db211

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

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