Merge pull request #72193 from jckarter/memory-lifetime-verifier-nondestructive-enum-addr

MemoryLifetimeVerifier: Treat only destructive enum_data_addr insns as writes.

Author: jckarter

docs/SIL.rst

@@ -7037,18 +7037,25 @@ unchecked_take_enum_data_addr
   // #U.DataCase must be a case of enum $U with data
   // %1 will be of address type $*T for the data type of case U.DataCase
 
-Invalidates an enum value, and takes the address of the payload for the given
-enum ``case`` in-place in memory. The referenced enum value is no longer valid,
-but the payload value referenced by the result address is valid and must be
-destroyed. It is undefined behavior if the referenced enum does not contain a
-value of the given ``case``. The result shares memory with the original enum
-value; the enum memory cannot be reinitialized as an enum until the payload has
-also been invalidated.
-
-(1.0 only)
-
-For the first payloaded case of an enum, ``unchecked_take_enum_data_addr``
-is guaranteed to have no side effects; the enum value will not be invalidated.
+Takes the address of the payload for the given enum ``case`` in-place in
+memory. It is undefined behavior if the referenced enum does not contain a
+value of the given ``case``. 
+
+The result shares memory with the original enum value.  If an enum declaration
+is unconditionally loadable (meaning it's loadable regardless of any generic
+parameters), and it has more than one case with an associated value, then it
+may embed the enum tag within the payload area. If this is the case, then
+`unchecked_take_enum_data_addr` will clear the tag from the payload,
+invalidating the referenced enum value, but leaving the
+payload value referenced by the result address valid. In these cases,
+the enum memory cannot be reinitialized as an enum until the payload has also
+been invalidated.
+
+If an enum has no more than one payload case, or if the declaration is ever
+address-only, then `unchecked_take_enum_data_addr` is guaranteed to be
+nondestructive, and the payload address can be accessed without invalidating
+the enum in these cases. The payload can be invalidated to invalidate the
+enum (assuming the enum does not have a `deinit` at the type level).
 
 select_enum
 ```````````

lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

@@ -757,11 +757,16 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
         // Note that despite the name, unchecked_take_enum_data_addr does _not_
         // "take" the payload of the Swift.Optional enum. This is a terrible
         // hack in SIL.
-        SILValue enumAddr = cast<UncheckedTakeEnumDataAddrInst>(&I)->getOperand();
-        int enumIdx = locations.getLocationIdx(enumAddr);
-        if (enumIdx >= 0)
-          requireBitsSet(bits, enumAddr, &I);
-        requireNoStoreBorrowLocation(enumAddr, &I);
+        auto enumInst = cast<UncheckedTakeEnumDataAddrInst>(&I);
+        // For some enums, projecting the enum data requires masking out
+        // embedded tag bits, which invalidates the value as an enum.
+        if (enumInst->isDestructive()) {
+          SILValue enumAddr = enumInst->getOperand();
+          int enumIdx = locations.getLocationIdx(enumAddr);
+          if (enumIdx >= 0)
+            requireBitsSet(bits, enumAddr, &I);
+          requireNoStoreBorrowLocation(enumAddr, &I);
+        }
         break;
       }
       case SILInstructionKind::DestroyAddrInst: {

test/Interpreter/moveonly_linkedlist_2_simple.swift

@@ -8,6 +8,7 @@
 // RUN: %FileCheck %s --check-prefix=CHECK-IR
 // RUN: %target-run-simple-swift(-parse-as-library -enable-builtin-module -enable-experimental-feature NoncopyableGenerics -enable-experimental-feature BorrowingSwitch -Xfrontend -sil-verify-all) | %FileCheck %s
 // RUN: %target-run-simple-swift(-O -parse-as-library -enable-builtin-module -enable-experimental-feature NoncopyableGenerics -enable-experimental-feature BorrowingSwitch -Xfrontend -sil-verify-all) | %FileCheck %s
+// RUN: %target-run-simple-swift(-O -parse-as-library -enable-builtin-module -enable-experimental-feature NoncopyableGenerics -enable-experimental-feature BorrowingSwitch -Xfrontend -sil-verify-all -Xfrontend -enable-ossa-modules) | %FileCheck %s
 
 // REQUIRES: executable_test
 

test/SIL/memory_lifetime_failures.sil

@@ -362,7 +362,6 @@ bb0(%0 :  @guaranteed $Optional<T>):
   return %res : $()
 }
 
-// CHECK: SIL memory lifetime failure in @test_store_borrow_take_enum: store-borrow location cannot be written
 sil [ossa] @test_store_borrow_take_enum : $@convention(thin) (@guaranteed Optional<T>) -> () {
 bb0(%0 :  @guaranteed $Optional<T>):
   %s = alloc_stack $Optional<T>