Merge pull request #32992 from atrick/prohibit-phi-args

Verify OSSA address phis.

Author: atrick

lib/SIL/Verifier/SILVerifier.cpp

@@ -909,25 +909,22 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     return InstNumbers[a] < InstNumbers[b];
   }
 
-  // FIXME: For sanity, address-type block args should be prohibited at all SIL
+  // FIXME: For sanity, address-type phis should be prohibited at all SIL
   // stages. However, the optimizer currently breaks the invariant in three
   // places:
   // 1. Normal Simplify CFG during conditional branch simplification
   //    (sneaky jump threading).
   // 2. Simplify CFG via Jump Threading.
   // 3. Loop Rotation.
   //
+  // BasicBlockCloner::canCloneInstruction and sinkAddressProjections is
+  // designed to avoid this issue, we just need to make sure all passes use it
+  // correctly.
   //
-  bool prohibitAddressBlockArgs() {
-    // If this function was deserialized from canonical SIL, this invariant may
-    // already have been violated regardless of this module's SIL stage or
-    // exclusivity enforcement level. Presumably, access markers were already
-    // removed prior to serialization.
-    if (F.wasDeserializedCanonical())
-      return false;
-
-    SILModule &M = F.getModule();
-    return M.getStage() == SILStage::Raw;
+  // Minimally, we must prevent address-type phis as long as access markers are
+  // preserved. A goal is to preserve access markers in OSSA.
+  bool prohibitAddressPhis() {
+    return F.hasOwnership();
   }
 
   void visitSILPhiArgument(SILPhiArgument *arg) {
@@ -943,9 +940,9 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
       }
     } else {
     }
-    if (arg->isPhiArgument() && prohibitAddressBlockArgs()) {
-      // As a property of well-formed SIL, we disallow address-type block
-      // arguments. Supporting them would prevent reliably reasoning about the
+    if (arg->isPhiArgument() && prohibitAddressPhis()) {
+      // As a property of well-formed SIL, we disallow address-type
+      // phis. Supporting them would prevent reliably reasoning about the
       // underlying storage of memory access. This reasoning is important for
       // diagnosing violations of memory access rules and supporting future
       // optimizations such as bitfield packing. Address-type block arguments

test/SIL/verifier-fail-address-phi-ossa.sil

@@ -0,0 +1,32 @@
+// RUN: %empty-directory(%t)
+// RUN: not --crash %target-sil-opt -enable-sil-verify-all %s 2> %t/err.txt
+// RUN: %FileCheck %s < %t/err.txt
+
+class Klass {}
+
+struct MyStruct {
+  @_hasStorage var k: Klass
+}
+
+// The SIL verifier must crash on address phis.
+//
+// CHECK: SIL verification failed: Block arguments cannot be addresses: !arg->getType().isAddress()
+sil [ossa] @handle_phi_address_nodes : $@convention(thin) (@owned Klass) -> () {
+bb0(%0 : @owned $Klass):
+  %1 = alloc_stack $MyStruct
+  %2 = struct_element_addr %1 : $*MyStruct, #MyStruct.k
+  store %0 to [init] %2 : $*Klass
+  cond_br undef, bb1, bb2
+
+bb1:
+  br bb3(%1 : $*MyStruct)
+
+bb2:
+  br bb3(%1 : $*MyStruct)
+
+bb3(%7 : $*MyStruct):
+  destroy_addr %7 : $*MyStruct
+  dealloc_stack %1 : $*MyStruct
+  %9999 = tuple()
+  return %9999 : $()
+}

test/SILOptimizer/semantic-arc-opts-canonical.sil

@@ -394,43 +394,6 @@ bb0(%0 : @guaranteed $(Klass, MyInt)):
   return %4 : $Builtin.Int32
 }
 
-// Make sure that we do not optimize this case.
-//
-// CHECK-LABEL: sil [ossa] @handle_phi_address_nodes : $@convention(thin) (@guaranteed Klass) -> () {
-// CHECK: load [copy]
-// CHECK: } // end sil function 'handle_phi_address_nodes'
-sil [ossa] @handle_phi_address_nodes : $@convention(thin) (@guaranteed Klass) -> () {
-bb0(%0 : @guaranteed $Klass):
-  %1 = enum $FakeOptional<Klass>, #FakeOptional.some!enumelt, %0 : $Klass
-  %2 = copy_value %1 : $FakeOptional<Klass>
-  %3 = copy_value %1 : $FakeOptional<Klass>
-
-  %4 = alloc_stack $FakeOptional<Klass>
-  store %2 to [init] %4 : $*FakeOptional<Klass>
-  %5 = alloc_stack $FakeOptional<Klass>
-  store %3 to [init] %5 : $*FakeOptional<Klass>
-
-  cond_br undef, bb1, bb2
-
-bb1:
-  br bb3(%4 : $*FakeOptional<Klass>)
-
-bb2:
-  br bb3(%5 : $*FakeOptional<Klass>)
-
-bb3(%6 : $*FakeOptional<Klass>):
-  %7 = load [copy] %6 : $*FakeOptional<Klass>
-  %8 = function_ref @fakeoptional_guaranteed_user : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()
-  apply %8(%7) : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()
-  destroy_value %7 : $FakeOptional<Klass>
-  destroy_addr %5 : $*FakeOptional<Klass>
-  dealloc_stack %5 : $*FakeOptional<Klass>
-  destroy_addr %4 : $*FakeOptional<Klass>
-  dealloc_stack %4 : $*FakeOptional<Klass>
-  %9999 = tuple()
-  return %9999 : $()
-}
-
 // CHECK-LABEL: sil [ossa] @switch_enum_test_loadcopy_no_default : $@convention(thin) (@owned FakeOptional<Builtin.NativeObject>) -> () {
 // CHECK-NOT: load [copy]
 // CHECK: load_borrow