Skip to content

Fix unchecked_take_enum_data_addr has side effects. #31157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix unchecked_take_enum_data_addr has side effects. #31157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lib/SILOptimizer/Utils/InstOptUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,6 @@ bool swift::isInstructionTriviallyDead(SILInstruction *inst) {
if (isa<DebugValueInst>(inst) || isa<DebugValueAddrInst>(inst))
return false;

// These invalidate enums so "write" memory, but that is not an essential
// operation so we can remove these if they are trivially dead.
if (isa<UncheckedTakeEnumDataAddrInst>(inst))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a special case for Optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return true;

if (!inst->mayHaveSideEffects())
return true;

Expand Down
13 changes: 13 additions & 0 deletions test/SILOptimizer/sil_combine.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3706,3 +3706,16 @@ bb0(%0 : $@thick SpecialEnum.Type):
%4 = struct $Bool (%3 : $Builtin.Int1)
return %4 : $Bool
}

// CHECK-LABEL: sil [ossa] @unchecked_take_enum_data_addr_not_dead : $@convention(thin) (@in FakeOptional<B>) -> ()
// CHECK: bb0(%0 : $*FakeOptional<B>):
// CHECK-NEXT: unchecked_take_enum_data_addr
// CHECK-NEXT: tuple
// CHECK-NEXT: return
// CHECK-LABEL end sil 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>):
%2 = unchecked_take_enum_data_addr %0 : $*FakeOptional<B>, #FakeOptional.some!enumelt
%4 = tuple ()
return %4 : $()
}