[SILOptimzer] Fix a crash caused by SILCombine mishandling inlined variables

This showed up on and off again on the source-compatibility testsuite project hummingbird.

The gist of the problem is that transformations may may not rewrite
the type of an inlined instance of a variable without also createing a
deep copy of the inlined function with a different name (and e.g., a
specialization suffix). Otherwise the modified inlined variable will
cause an inconsistency when later compiler passes try to create the
abstract declaration of that inlined function as there would be
conflicting declarations for that variable.

Since SILDebugScope isn't yet available in the SwiftCompilerSources
this fix just drop these variables, but it would be absolutely
possible to preserve them by using the same mechanism that SILCloner
uses to create a deep copy of the inlined function scopes.

rdar://163167975

Author: adrian-prantl

SwiftCompilerSources/Sources/Optimizer/InstructionSimplification/SimplifyAllocStack.swift

@@ -225,6 +225,17 @@ private extension AllocStackInst {
           source: newAlloc, sourceFormalType: concreteFormalType,
           destination: ucca.destination, targetFormalType: ucca.targetFormalType)
         context.erase(instruction: ucca)
+      case let dv as DebugValueInst:
+        if dv.location.isInlined {
+          // We cannot change the type of an inlined instance of a variable
+          // without renaming the inlined function to get a unique
+          // specialization suffix (prior art exists in
+          // SILCloner::remapFunction()).
+          // For now, just remove affected inlined variables.
+          use.set(to: Undef.get(type: type, context), context)
+        } else {
+          use.set(to: newAlloc, context)
+        }
       default:
         use.set(to: newAlloc, context)
       }

include/swift/SIL/TypeSubstCloner.h

@@ -423,14 +423,15 @@ class TypeSubstCloner : public SILClonerWithScopes<ImplClass> {
     // and types. We check if the function is called with non-identity substitutions
     // to decide whether it's necessary to clone a unique copy of the function
     // declaration with the substitutions applied for the debug info.
-    if (SubsMap.isIdentity())
+    if (SubsMap.isIdentity() &&
+        !SubsMap.getRecursiveProperties().hasTypeParameter())
       return ParentFunction;
 
     // Note that mapReplacementTypesOutOfContext() can't do anything for
     // opened existentials, and since archetypes can't be mangled, ignore
     // this case for now.
     if (SubsMap.getRecursiveProperties().hasLocalArchetype())
-      return ParentFunction;
+      SubsMap = {};
 
     // Clone the function with the substituted type for the debug info.
     Mangle::GenericSpecializationMangler Mangler(M.getASTContext(), ParentFunction,

lib/SIL/Verifier/SILVerifier.cpp

@@ -1658,12 +1658,48 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
       require(lhs == rhs ||
               (lhs.isAddress() && lhs.getObjectType() == rhs) ||
               (DebugVarTy.isAddress() && lhs == rhs.getObjectType()) ||
-
               // When cloning SIL (e.g. in LoopUnroll) local archetypes are uniqued
               // and therefore distinct in cloned instructions.
               (lhs.hasLocalArchetype() && rhs.hasLocalArchetype()),
-              "Two variables with different type but same scope!");
-    }
+              "Two variables with different type but same scope");
+    }
+
+    // Check that all inlined function arguments agree on their types.
+#ifdef EXPENSIVE_VERIFIER_CHECKS
+    if (unsigned ArgNo = varInfo->ArgNo)
+      if (varInfo->Scope)
+        if (SILFunction *Fn = varInfo->Scope->getInlinedFunction()) {
+          using ArgMap = llvm::StringMap<llvm::SmallVector<SILType, 8>>;
+          static ArgMap DebugArgs;
+          llvm::StringRef Key = Fn->getName();
+          if (!Key.empty()) {
+            auto [It, Inserted] = DebugArgs.insert({Key, {}});
+            auto &CachedArgs = It->second;
+            if (Inserted || (!Inserted && (CachedArgs.size() < ArgNo)) ||
+                (!Inserted && !CachedArgs[ArgNo - 1])) {
+              if (CachedArgs.size() < ArgNo)
+                CachedArgs.resize(ArgNo);
+              CachedArgs[ArgNo - 1] = DebugVarTy;
+            } else {
+              SILType CachedArg = CachedArgs[ArgNo - 1];
+              auto lhs = CachedArg.removingMoveOnlyWrapper();
+              auto rhs = DebugVarTy.removingMoveOnlyWrapper();
+              if (lhs != rhs) {
+                llvm::errs() << "***** " << varInfo->Name << "\n";
+                lhs.dump();
+                rhs.dump();
+                Fn->dump();
+              }
+              require(
+                  lhs == rhs ||
+                      (lhs.isAddress() && lhs.getObjectType() == rhs) ||
+                      (DebugVarTy.isAddress() && lhs == rhs.getObjectType()) ||
+                      (lhs.hasLocalArchetype() && rhs.hasLocalArchetype()),
+                  "Conflicting types for function argument!");
+            }
+          }
+        }
+#endif
 
     // Check debug info expression
     if (const auto &DIExpr = varInfo->DIExpr) {

test/SILOptimizer/simplify_alloc_stack.sil

@@ -312,6 +312,28 @@ bb0(%0 : $*T, %1 : @owned $T):
   return %r
 }
 
+// CHECK-LABEL: sil [ossa] @replace_existential_with_concrete_type3 :
+// CHECK:         [[S:%.*]] = alloc_stack $T
+// CHECK-NOT:     init_existential_addr
+// CHECK-NOT:     open_existential_addr
+// CHECK:         debug_value undef : $*any P, let, name "value1"
+// CHECK:         destroy_addr [[S]]
+// CHECK:       } // end sil function 'replace_existential_with_concrete_type3'
+sil_scope 1 { loc "a.swift":1:1 parent @replace_existential_with_concrete_type3 : $@convention(thin) (@owned T) -> () }
+sil_scope 2 { loc "inlined.swift":1:1 parent @$inlined :  $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> () inlined_at 1 }
+sil [ossa] @replace_existential_with_concrete_type3 : $@convention(thin) (@owned T) -> () {
+bb0(%0 : @owned $T):
+  %5 = alloc_stack $any P
+  debug_value %5, let, name "value1", loc "inlined.swift":1:1, scope 2
+  %6 = init_existential_addr %5, $T
+  store %0 to [init] %6
+  %8 = open_existential_addr mutable_access %5 to $*@opened("83DE9694-7315-11E8-955C-ACDE48001122", P) Self
+  destroy_addr %8
+  dealloc_stack %5
+  %r = tuple ()
+  return %r
+}
+
 // CHECK-LABEL: sil [ossa] @replace_existential_with_archetype :
 // CHECK:         [[S:%.*]] = alloc_stack $@opened("82105EE0-DCB0-11E5-865D-C8E0EB309913", any P) Self
 // CHECK-NOT:     init_existential_addr