[opt-remark] Handle inline locations correctly.

Specifically, now we properly identify the SILLocation for the final inlined
call site of a Source Location and put the remark there instead of in the
callee.

Author: gottesmm

include/swift/SIL/SILDebugScope.h

@@ -66,6 +66,28 @@ class SILDebugScope : public SILAllocated<SILDebugScope> {
   /// into.
   SILFunction *getParentFunction() const;
 
+  /// If this is a debug scope associated with an inlined call site, return the
+  /// SILLocation associated with the call site resulting from the final
+  /// inlining.
+  ///
+  /// This allows one to emit diagnostics based off of inlined code's final
+  /// location in the function that was inlined into.
+  SILLocation getOutermostInlineLocation() const {
+    if (!InlinedCallSite)
+      return SILLocation::invalid();
+
+    auto *scope = this;
+    do {
+      scope = scope->InlinedCallSite;
+    } while (scope->InlinedCallSite);
+
+    SILLocation callSite = scope->Loc;
+    if (callSite.isNull() || !callSite.isASTNode())
+      return SILLocation::invalid();
+
+    return callSite;
+  }
+
   void print(SourceManager &SM, llvm::raw_ostream &OS = llvm::errs(),
              unsigned Indent = 0) const;
 

lib/SIL/Utils/OptimizationRemark.cpp

@@ -152,6 +152,8 @@ static SourceLoc inferOptRemarkSearchForwards(SILInstruction &i) {
   for (auto &inst :
        llvm::make_range(std::next(i.getIterator()), i.getParent()->end())) {
     auto newLoc = inst.getLoc().getSourceLoc();
+    if (auto inlinedLoc = inst.getDebugScope()->getOutermostInlineLocation())
+      newLoc = inlinedLoc.getSourceLoc();
     if (newLoc.isValid())
       return newLoc;
   }
@@ -167,7 +169,9 @@ 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))
+    if (auto inlinedLoc = inst.getDebugScope()->getOutermostInlineLocation())
+      loc = inlinedLoc;
+    if (!loc.getSourceLoc().isValid())
       continue;
 
     auto range = loc.getSourceRange();
@@ -180,31 +184,35 @@ static SourceLoc inferOptRemarkSearchBackwards(SILInstruction &i) {
 
 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;
+  // 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.
+  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();
+  }
 
   // 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;
+    return SourceLoc();
   case SourceLocInferenceBehavior::ForwardScanOnly: {
     SourceLoc newLoc = inferOptRemarkSearchForwards(i);
     if (newLoc.isValid())
       return newLoc;
-    return loc;
+    return SourceLoc();
   }
   case SourceLocInferenceBehavior::BackwardScanOnly: {
     SourceLoc newLoc = inferOptRemarkSearchBackwards(i);
     if (newLoc.isValid())
       return newLoc;
-    return loc;
+    return SourceLoc();
   }
   case SourceLocInferenceBehavior::ForwardThenBackward: {
     SourceLoc newLoc = inferOptRemarkSearchForwards(i);
@@ -213,7 +221,7 @@ SourceLoc swift::OptRemark::inferOptRemarkSourceLoc(
     newLoc = inferOptRemarkSearchBackwards(i);
     if (newLoc.isValid())
       return newLoc;
-    return loc;
+    return SourceLoc();
   }
   case SourceLocInferenceBehavior::BackwardThenForward: {
     SourceLoc newLoc = inferOptRemarkSearchBackwards(i);
@@ -222,7 +230,7 @@ SourceLoc swift::OptRemark::inferOptRemarkSourceLoc(
     newLoc = inferOptRemarkSearchForwards(i);
     if (newLoc.isValid())
       return newLoc;
-    return loc;
+    return SourceLoc();
   }
   }
 

lib/SILOptimizer/Transforms/OptRemarkGenerator.cpp

@@ -42,7 +42,7 @@ static llvm::cl::opt<bool> ForceVisitImplicitAutogeneratedFunctions(
     llvm::cl::init(false));
 
 //===----------------------------------------------------------------------===//
-//                                  Utility
+//                           Value To Decl Inferrer
 //===----------------------------------------------------------------------===//
 
 namespace {
@@ -176,15 +176,21 @@ bool ValueToDeclInferrer::infer(
         continue;
 
       if (auto *dvi = dyn_cast<DebugValueInst>(use->getUser())) {
-        if (auto *decl = dvi->getDecl()) {
-          std::string msg;
-          {
-            llvm::raw_string_ostream stream(msg);
-            printNote(stream, decl);
+        // Check if our debug_value has a decl and was not inlined into the
+        // current function.
+        if (auto *scope = dvi->getDebugScope()) {
+          if (!scope->InlinedCallSite) {
+            if (auto *decl = dvi->getDecl()) {
+              std::string msg;
+              {
+                llvm::raw_string_ostream stream(msg);
+                printNote(stream, decl);
+              }
+              resultingInferredDecls.push_back(
+                  Argument({keyKind, "InferredValue"}, std::move(msg), decl));
+              foundDeclFromUse = true;
+            }
           }
-          resultingInferredDecls.push_back(
-              Argument({keyKind, "InferredValue"}, std::move(msg), decl));
-          foundDeclFromUse = true;
         }
       }
     }

test/SILOptimizer/opt-remark-generator.swift

@@ -231,3 +231,29 @@ func inoutKlassQuestionCastArgument2(x: inout Klass?) -> SubKlass? {
     return x as? SubKlass // expected-remark {{retain of type 'Klass'}}
                           // expected-note @-2 {{of 'x.some'}}
 }
+
+// We should have 1x rr remark here on calleeX for storing it into the array to
+// print. Release is from the array. We don't pattern match it due to the actual
+// underlying Array type name changing under the hood in between platforms.
+@inline(__always)
+func alwaysInlineCallee(_ calleeX: Klass) {
+    print(calleeX) // expected-remark @:5 {{retain of type 'Klass'}}
+                   // expected-note @-2:27 {{of 'calleeX'}}
+                   // expected-remark @-2:18 {{release of type}}
+}
+
+// We should have 3x rr remarks here on callerX and none on calleeX.  All of the
+// releases are for the temporary array that we pass into print.
+//
+// TODO: Should we print out as notes the whole inlined call stack?
+func alwaysInlineCaller(_ callerX: Klass) {
+    alwaysInlineCallee(callerX) // expected-remark @:5 {{retain of type 'Klass'}}
+                                // expected-note @-2:27 {{of 'callerX'}}
+                                // expected-remark @-2:31 {{release of type}}
+    print(callerX)              // expected-remark @:5 {{retain of type 'Klass'}}
+                                // expected-note @-5:27 {{of 'callerX'}}
+                                // expected-remark @-2:18 {{release of type}}
+    alwaysInlineCallee(callerX) // expected-remark @:5 {{retain of type 'Klass'}}
+                                // expected-note @-8:27 {{of 'callerX'}}
+                                // expected-remark @-2:31 {{release of type}}
+}