[opt-remark] Add support for simple object projections.

gottesmm · gottesmm · commit 0bd6a58faed3 · 2020-07-27T12:35:00.000-07:00
This ensures that we are able to properly look through struct_extract,
tuple_extract in cases where we have an aggregate with multiple non-trivial
subtypes (implying it is not-rc identical with those sub-types).

The end result is that we now emit good diagnostics for things like this:

```
func returnStructWithOwnerOwner(x: StructWithOwner) -&gt; Klass {
    return x.owner // expected-remark {{retain}}
                   // expected-note @-7:33 {{of 'x.owner'}}
}
```
diff --git a/include/swift/SIL/Projection.h b/include/swift/SIL/Projection.h
@@ -247,7 +247,8 @@ class Projection {
 
   Projection &operator=(Projection &&P) = default;
 
-  bool isValid() const { return Value.isValid(); }
+  bool isValid() const { return bool(*this); }
+  operator bool() const { return Value.isValid(); }
 
   /// Convenience method for getting the underlying index. Assumes that this
   /// projection is valid. Otherwise it asserts.
diff --git a/lib/SILOptimizer/Transforms/OptRemarkGenerator.cpp b/lib/SILOptimizer/Transforms/OptRemarkGenerator.cpp
@@ -9,6 +9,13 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+///
+/// \file
+///
+/// In this pass, we define the opt-remark-generator, a simple SILVisitor that
+/// attempts to infer opt-remarks for the user using heuristics.
+///
+//===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "sil-opt-remark-gen"
 
@@ -33,32 +40,95 @@ using namespace swift;
 
 namespace {
 
-/// Given a value, call \p funcPassedInferredArgs for each associated
-/// ValueDecl that is associated with \p value. All created Arguments are
-/// passed the same StringRef. To stop iteration, return false in \p
-/// funcPassedInferedArgs.
-///
-/// NOTE: the function may be called multiple times if the optimizer joined
-/// two live ranges and thus when iterating over value's users we see multiple
-/// debug_value operations.
 struct ValueToDeclInferrer {
   using Argument = OptRemark::Argument;
   using ArgumentKeyKind = OptRemark::ArgumentKeyKind;
 
+  RCIdentityFunctionInfo &rcfi;
+  SmallVector<std::pair<SILType, Projection>, 32> accessPath;
+
+  ValueToDeclInferrer(RCIdentityFunctionInfo &rcfi) : rcfi(rcfi) {}
+
+  /// Given a value, attempt to infer a conservative list of decls that the
+  /// passed in value could be referring to. This is done just using heuristics
   bool infer(ArgumentKeyKind keyKind, SILValue value,
-             function_ref<bool(Argument)> funcPassedInferedArgs);
+             SmallVectorImpl<Argument> &resultingInferredDecls);
+
+  /// Print out a note to \p stream that beings at decl and then consumes the
+  /// accessPath we computed for decl producing a segmented access path, e.x.:
+  /// "of 'x.lhs.ivar'".
+  void printNote(llvm::raw_string_ostream &stream, const ValueDecl *decl);
 };
 
 } // anonymous namespace
 
-static void printNote(llvm::raw_string_ostream &stream, const ValueDecl *decl) {
-  stream << "of '" << decl->getBaseName() << "'";
+void ValueToDeclInferrer::printNote(llvm::raw_string_ostream &stream,
+                                    const ValueDecl *decl) {
+  stream << "of '" << decl->getBaseName();
+  for (auto &pair : accessPath) {
+    auto baseType = pair.first;
+    auto &proj = pair.second;
+    stream << ".";
+
+    // WARNING: This must be kept insync with isSupportedProjection!
+    switch (proj.getKind()) {
+    case ProjectionKind::Upcast:
+      stream << "upcast<" << proj.getCastType(baseType) << ">";
+      continue;
+    case ProjectionKind::RefCast:
+      stream << "refcast<" << proj.getCastType(baseType) << ">";
+      continue;
+    case ProjectionKind::BitwiseCast:
+      stream << "bitwise_cast<" << proj.getCastType(baseType) << ">";
+      continue;
+    case ProjectionKind::Struct:
+      stream << proj.getVarDecl(baseType)->getBaseName();
+      continue;
+    case ProjectionKind::Tuple:
+      stream << proj.getIndex();
+      continue;
+    case ProjectionKind::Enum:
+      stream << proj.getEnumElementDecl(baseType)->getBaseName();
+      continue;
+    // Object -> Address projections can never be looked through.
+    case ProjectionKind::Class:
+    case ProjectionKind::Box:
+    case ProjectionKind::Index:
+    case ProjectionKind::TailElems:
+      llvm_unreachable(
+          "Object -> Address projection should never be looked through!");
+    }
+
+    llvm_unreachable("Covered switch is not covered?!");
+  }
+
+  accessPath.clear();
+  stream << "'";
+}
+
+// WARNING: This must be kept insync with ValueToDeclInferrer::printNote(...).
+static SingleValueInstruction *isSupportedProjection(Projection p, SILValue v) {
+  switch (p.getKind()) {
+  case ProjectionKind::Upcast:
+  case ProjectionKind::RefCast:
+  case ProjectionKind::BitwiseCast:
+  case ProjectionKind::Struct:
+  case ProjectionKind::Tuple:
+  case ProjectionKind::Enum:
+    return cast<SingleValueInstruction>(v);
+    // Object -> Address projections can never be looked through.
+  case ProjectionKind::Class:
+  case ProjectionKind::Box:
+  case ProjectionKind::Index:
+  case ProjectionKind::TailElems:
+    return nullptr;
+  }
+  llvm_unreachable("Covered switch is not covered?!");
 }
 
 bool ValueToDeclInferrer::infer(
     ArgumentKeyKind keyKind, SILValue value,
-    function_ref<bool(Argument)> funcPassedInferedArgs) {
-
+    SmallVectorImpl<Argument> &resultingInferredDecls) {
   // This is a linear IR traversal using a 'falling while loop'. That means
   // every time through the loop we are trying to handle a case before we hit
   // the bottom of the while loop where we always return true (since we did not
@@ -73,8 +143,9 @@ bool ValueToDeclInferrer::infer(
           llvm::raw_string_ostream stream(msg);
           printNote(stream, decl);
         }
-        return funcPassedInferedArgs(
+        resultingInferredDecls.push_back(
             Argument({keyKind, "InferredValue"}, std::move(msg), decl));
+        return true;
       }
 
     if (auto *ga = dyn_cast<GlobalAddrInst>(value))
@@ -84,13 +155,14 @@ bool ValueToDeclInferrer::infer(
           llvm::raw_string_ostream stream(msg);
           printNote(stream, decl);
         }
-        if (!funcPassedInferedArgs(
-                Argument({keyKind, "InferredValue"}, std::move(msg), decl)))
-          return false;
+        resultingInferredDecls.push_back(
+            Argument({keyKind, "InferredValue"}, std::move(msg), decl));
+        return true;
       }
 
     // Then visit our users and see if we can find a debug_value that provides
     // us with a decl we can use to construct an argument.
+    bool foundDeclFromUse = false;
     for (auto *use : value->getUses()) {
       // Skip type dependent uses.
       if (use->isTypeDependent())
@@ -103,12 +175,14 @@ bool ValueToDeclInferrer::infer(
             llvm::raw_string_ostream stream(msg);
             printNote(stream, decl);
           }
-          if (!funcPassedInferedArgs(
-                  Argument({keyKind, "InferredValue"}, std::move(msg), decl)))
-            return false;
+          resultingInferredDecls.push_back(
+              Argument({keyKind, "InferredValue"}, std::move(msg), decl));
+          foundDeclFromUse = true;
         }
       }
     }
+    if (foundDeclFromUse)
+      return true;
 
     // At this point, we could not infer any argument. See if we can look
     // through loads.
@@ -121,6 +195,14 @@ bool ValueToDeclInferrer::infer(
       continue;
     }
 
+    if (auto proj = Projection(value)) {
+      if (auto *projInst = isSupportedProjection(proj, value)) {
+        value = projInst->getOperand(0);
+        accessPath.emplace_back(value->getType(), proj);
+        continue;
+      }
+    }
+
     // If we reached this point, we finished falling through the loop and return
     // true.
     return true;
@@ -136,7 +218,6 @@ namespace {
 struct OptRemarkGeneratorInstructionVisitor
     : public SILInstructionVisitor<OptRemarkGeneratorInstructionVisitor> {
   SILModule &mod;
-  RCIdentityFunctionInfo &rcfi;
   OptRemark::Emitter ORE;
 
   /// A class that we use to infer the decl that is associated with a
@@ -145,7 +226,7 @@ struct OptRemarkGeneratorInstructionVisitor
 
   OptRemarkGeneratorInstructionVisitor(SILFunction &fn,
                                        RCIdentityFunctionInfo &rcfi)
-      : mod(fn.getModule()), rcfi(rcfi), ORE(DEBUG_TYPE, fn) {}
+      : mod(fn.getModule()), ORE(DEBUG_TYPE, fn), valueToDeclInferrer(rcfi) {}
 
   void visitStrongRetainInst(StrongRetainInst *sri);
   void visitStrongReleaseInst(StrongReleaseInst *sri);
@@ -160,13 +241,9 @@ void OptRemarkGeneratorInstructionVisitor::visitStrongRetainInst(
     StrongRetainInst *sri) {
   ORE.emit([&]() {
     using namespace OptRemark;
-    SILValue root = rcfi.getRCIdentityRoot(sri->getOperand());
     SmallVector<Argument, 8> inferredArgs;
-    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note, root,
-                                               [&](Argument arg) {
-                                                 inferredArgs.push_back(arg);
-                                                 return true;
-                                               });
+    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note,
+                                               sri->getOperand(), inferredArgs);
     (void)foundArgs;
 
     // Retains begin a lifetime scope so we infer scan forward.
@@ -185,13 +262,9 @@ void OptRemarkGeneratorInstructionVisitor::visitStrongReleaseInst(
   ORE.emit([&]() {
     using namespace OptRemark;
     // Releases end a lifetime scope so we infer scan backward.
-    SILValue root = rcfi.getRCIdentityRoot(sri->getOperand());
     SmallVector<Argument, 8> inferredArgs;
-    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note, root,
-                                               [&](Argument arg) {
-                                                 inferredArgs.push_back(arg);
-                                                 return true;
-                                               });
+    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note,
+                                               sri->getOperand(), inferredArgs);
     (void)foundArgs;
 
     auto remark = RemarkMissed("memory", *sri,
@@ -208,15 +281,10 @@ void OptRemarkGeneratorInstructionVisitor::visitRetainValueInst(
     RetainValueInst *rvi) {
   ORE.emit([&]() {
     using namespace OptRemark;
-    SILValue root = rcfi.getRCIdentityRoot(rvi->getOperand());
     SmallVector<Argument, 8> inferredArgs;
-    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note, root,
-                                               [&](Argument arg) {
-                                                 inferredArgs.push_back(arg);
-                                                 return true;
-                                               });
+    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note,
+                                               rvi->getOperand(), inferredArgs);
     (void)foundArgs;
-
     // Retains begin a lifetime scope, so we infer scan forwards.
     auto remark = RemarkMissed("memory", *rvi,
                                SourceLocInferenceBehavior::ForwardScanOnly)
@@ -232,13 +300,9 @@ void OptRemarkGeneratorInstructionVisitor::visitReleaseValueInst(
     ReleaseValueInst *rvi) {
   ORE.emit([&]() {
     using namespace OptRemark;
-    SILValue root = rcfi.getRCIdentityRoot(rvi->getOperand());
     SmallVector<Argument, 8> inferredArgs;
-    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note, root,
-                                               [&](Argument arg) {
-                                                 inferredArgs.push_back(arg);
-                                                 return true;
-                                               });
+    bool foundArgs = valueToDeclInferrer.infer(ArgumentKeyKind::Note,
+                                               rvi->getOperand(), inferredArgs);
     (void)foundArgs;
 
     // Releases end a lifetime scope so we infer scan backward.
diff --git a/test/SILOptimizer/opt-remark-generator-yaml.swift b/test/SILOptimizer/opt-remark-generator-yaml.swift
@@ -0,0 +1,76 @@
+// RUN: %target-swiftc_driver -O -Rpass-missed=sil-opt-remark-gen -Xllvm -sil-disable-pass=FunctionSignatureOpts -emit-sil %s -o /dev/null -Xfrontend -verify
+
+// RUN: %empty-directory(%t)
+// RUN: %target-swiftc_driver -wmo -O -Xllvm -sil-disable-pass=FunctionSignatureOpts -emit-sil -save-optimization-record=yaml  -save-optimization-record-path %t/note.yaml %s -o /dev/null && %FileCheck --input-file=%t/note.yaml %s
+
+// This file is testing out the basic YAML functionality to make sure that it
+// works without burdening opt-remark-generator-yaml.swift with having to update all
+// of the yaml test cases everytime new code is added.
+
+// CHECK: --- !Missed
+// CHECK-NEXT: Pass:            sil-opt-remark-gen
+// CHECK-NEXT: Name:            sil.memory
+// CHECK-NEXT: DebugLoc:        { File: '{{.*}}opt-remark-generator-yaml.swift',
+// CHECK-NEXT:                    Line: 63, Column: 5 }
+// CHECK-NEXT: Function:        'getGlobal()'
+// CHECK-NEXT: Args:
+// CHECK-NEXT:   - String:          retain
+// CHECK-NEXT:   - InferredValue:   'of ''global'''
+// CHECK-NEXT:     DebugLoc:        { File: '{{.*}}opt-remark-generator-yaml.swift',
+// CHECK-NEXT:                        Line: 59, Column: 12 }
+// CHECK-NEXT: ...
+// CHECK-NEXT: --- !Missed
+// CHECK-NEXT: Pass:            sil-opt-remark-gen
+// CHECK-NEXT: Name:            sil.memory
+// CHECK-NEXT: DebugLoc:        { File: '{{.*}}opt-remark-generator-yaml.swift',
+// CHECK-NEXT:                    Line: 71, Column: 5 }
+// CHECK-NEXT: Function:        'useGlobal()'
+// CHECK-NEXT: Args:
+// CHECK-NEXT:   - String:          retain
+// CHECK-NEXT:   - InferredValue:   'of ''x'''
+// CHECK-NEXT:     DebugLoc:        { File: '{{.*}}opt-remark-generator-yaml.swift',
+// CHECK-NEXT:                        Line: 68, Column: 9 }
+// CHECK-NEXT: ...
+// CHECK-NEXT: --- !Missed
+// CHECK-NEXT: Pass:            sil-opt-remark-gen
+// CHECK-NEXT: Name:            sil.memory
+// CHECK-NEXT: DebugLoc:        { File: '{{.*}}opt-remark-generator-yaml.swift',
+// CHECK-NEXT:                    Line: 71, Column: 12 }
+// CHECK-NEXT: Function:        'useGlobal()'
+// CHECK-NEXT: Args:
+// CHECK-NEXT:   - String:          release
+
+// CHECK-NEXT: ...
+// CHECK-NEXT: --- !Missed
+// CHECK-NEXT: Pass:            sil-opt-remark-gen
+// CHECK-NEXT: Name:            sil.memory
+// CHECK-NEXT: DebugLoc:        { File: '{{.*}}opt-remark-generator-yaml.swift',
+// CHECK-NEXT:                    Line: 71, Column: 12 }
+// CHECK-NEXT: Function:        'useGlobal()'
+// CHECK-NEXT: Args:
+// CHECK-NEXT:   - String:          release
+// CHECK-NEXT:   - InferredValue:   'of ''x'''
+// CHECK-NEXT:     DebugLoc:        { File: '{{.*}}opt-remark-generator-yaml.swift',
+// CHECK-NEXT:                        Line: 68, Column: 9 }
+// CHECK-NEXT: ...
+
+public class Klass {}
+
+public var global = Klass()
+
+@inline(never)
+public func getGlobal() -> Klass {
+    return global // expected-remark @:5 {{retain}}
+                  // expected-note @-5:12 {{of 'global'}}
+}
+
+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 {{retain}}
+             // expected-note @-4:9 {{of 'x'}}
+             // expected-remark @-2:12 {{release}}
+             // expected-remark @-3:12 {{release}}
+             // expected-note @-7:9 {{of 'x'}}
+}
diff --git a/test/SILOptimizer/opt-remark-generator.swift b/test/SILOptimizer/opt-remark-generator.swift
