Fix unchecked_take_enum_data_addr has side effects.

[zoecarver]

Don't always return true (is trivially dead) for unchecked_take_enum_data_addr. It destroys the address operand so, it's not always trivially dead.

Fixes issue described in #31077.

[gottesmm]

@zoecarver This needs a test case showing the crash.

[zoecarver]

@gottesmm this doesn't crash without the mandatory combine patch (because without ownership the verifier doesn't care that it's not initialized) but, you can see that it's removed (incorrectly) in the non-OSSA SILCombine test I added.

[zoecarver]

I can also get it to crash with mandatory inlining if that's better.

[gottesmm]

@zoecarver I'd be down with mandatory inlining.

[zoecarver]

Sorry, I didn't look closely enough. I can't get mandatory inlining to crash. I think SILCombine is the best we've got.

[zoecarver]

Actually, I forgot! We support OSSA in SILCombine now :D

The test now crashes (on master).

[zoecarver]

@gottesmm friendly ping :)

[gottesmm]

I would add a special case here for Optional. UncheckedTakeEnumDataAddrInst is only writable for certain multi-payload cases where to extract the payload one needs to "open" the enum. This is not true for optional. Also, even so, I don't understand what the crasher is here. Whats the crash specifically, can you post the error?

[zoecarver]

Here's the error:

SIL memory lifetime failure in @unchecked_take_enum_data_addr_not_dead: memory is initialized at function return but shouldn't
memory location: %0 = argument of bb0 : $*FakeOptional<B>
at instruction:   return %1 : $()                                 // id: %2

in function:
// unchecked_take_enum_data_addr_not_dead
sil [ossa] @unchecked_take_enum_data_addr_not_dead : $@convention(thin) (@in FakeOptional<B>) -> () {
bb0(%0 : $*FakeOptional<B>):
  %1 = tuple ()                                   // user: %2
  return %1 : $()                                 // id: %2
} // end sil function 'unchecked_take_enum_data_addr_not_dead'

Because unchecked_take_enum_data_addr takes (or destroys) the operand. When the instruction is removed, the operand is still initialized.

[zoecarver]

I'll add a special case for Optional.

[zoecarver]

Actually, I can't add a special case for optional. It's the same problem (not sure why I thought it wouldn't be at first...) but, it's still an @in address. It needs to be destroyed somehow.

[zoecarver]

We could promote it to a destroy_address if the uses are empty. But, that's probably better done in sil-combine (or maybe DCE).

[gottesmm]

@zoecarver now that I am looking at this, I don't think this is the correct fix. I think more likely there was some sort of destroy_addr here or something like that. This instruction's write semantics really shouldn't be visible to the user in this way. I would go back and see what used to be at the end of the unchecked_take_enum_data.

[zoecarver]

@gottesmm I'm not sure I completely understand what you mean. Unless I completely misunderstand, unchecked_take_enum_data_addr does have side effects, it destroys the address. So, it's not dead.

Here's the original king fisher reproducer. There's no destroy_addr:
Before

bb10:                                             // Preds: bb0
  %76 = alloc_stack $KingfisherOptionsInfoItem    // users: %91, %85, %78, %77
  copy_addr %13 to [initialization] %76 : $*KingfisherOptionsInfoItem // id: %77
  %78 = unchecked_take_enum_data_addr %76 : $*KingfisherOptionsInfoItem, #KingfisherOptionsInfoItem.transition!enumelt // user: %79
  %79 = load [trivial] %78 : $*ImageTransition
  switch_enum_addr %14 : $*KingfisherOptionsInfoItem, case #KingfisherOptionsInfoItem.transition!enumelt: bb11, default bb12 // id: %80

bb11:                                             // Preds: bb10
  %81 = unchecked_take_enum_data_addr %14 : $*KingfisherOptionsInfoItem, #KingfisherOptionsInfoItem.transition!enumelt // user: %82
  %82 = load [trivial] %81 : $*ImageTransition
  %83 = integer_literal $Builtin.Int1, -1         // user: %84
  %84 = struct $Bool (%83 : $Builtin.Int1)        // user: %90
  dealloc_stack %76 : $*KingfisherOptionsInfoItem // id: %85
  destroy_addr %13 : $*KingfisherOptionsInfoItem  // id: %86
  dealloc_stack %8 : $*(KingfisherOptionsInfoItem, KingfisherOptionsInfoItem) // id: %87
  dealloc_stack %6 : $*KingfisherOptionsInfoItem  // id: %88
  dealloc_stack %4 : $*KingfisherOptionsInfoItem  // id: %89
  br bb65(%84 : $Bool)                            // id: %90

bb12:                                             // Preds: bb10
  dealloc_stack %76 : $*KingfisherOptionsInfoItem // id: %91
  br bb64                                         // id: %92

After

bb10:                                             // Preds: bb0
  %76 = alloc_stack $KingfisherOptionsInfoItem    // users: %87, %81, %77
  copy_addr %13 to [initialization] %76 : $*KingfisherOptionsInfoItem // id: %77
  switch_enum_addr %14 : $*KingfisherOptionsInfoItem, case #KingfisherOptionsInfoItem.transition!enumelt: bb11, default bb12 // id: %78

bb11:                                             // Preds: bb10
  %79 = integer_literal $Builtin.Int1, -1         // user: %80
  %80 = struct $Bool (%79 : $Builtin.Int1)        // user: %86
  dealloc_stack %76 : $*KingfisherOptionsInfoItem // id: %81
  destroy_addr %13 : $*KingfisherOptionsInfoItem  // id: %82
  dealloc_stack %8 : $*(KingfisherOptionsInfoItem, KingfisherOptionsInfoItem) // id: %83
  dealloc_stack %6 : $*KingfisherOptionsInfoItem  // id: %84
  dealloc_stack %4 : $*KingfisherOptionsInfoItem  // id: %85
  br bb65(%80 : $Bool)                            // id: %86

bb12:                                             // Preds: bb10
  dealloc_stack %76 : $*KingfisherOptionsInfoItem // id: %87
  br bb64                                         // id: %88

[gottesmm]

@zoecarver this is a question around the lifetime verifier and what we are verifying. The memory lifetime verifier is not the end all be all definition of correctness. It is just enforcing what it knows and is incomplete.

I would argue that the right thing here is to look at why we don't have a destroy_addr. I think that is the right thing to ask here. Later on after ossa, we will just eliminate the destroy_addr. But in OSSA, we will want it.

[zoecarver]

@gottesmm OK, I see what you mean. I'll look into it. Probably a SILGen change.

[zoecarver]

Here's the issue: https://swift.godbolt.org/z/9n3PwS

Essentially when we have a protocol and a trivial type in an enum and we try to switch over them, we destroy the protocol but not the trivial type. I think we should be destroying the enum value and not the enum "data". WDYT?

Side note: there are some missed optimizations with copy_addr in the optimized version of that example. I'll try to fix those after I get this fixed.

[zoecarver]

Closing in favor of #31779.