Skip to content

Remove a stale UnenforcedAccess marker from SILGenPattern. #22452

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

Merged
merged 1 commit into from
Feb 14, 2019
Merged

Remove a stale UnenforcedAccess marker from SILGenPattern. #22452

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
34 changes: 6 additions & 28 deletions lib/SILGen/SILGenPattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1920,12 +1920,7 @@ void PatternMatchEmission::emitEnumElementObjectDispatch(
ManagedValue boxedValue =
SGF.B.createProjectBox(loc, origCMV.getFinalManagedValue(), 0);
eltTL = &SGF.getTypeLowering(boxedValue.getType());

// TODO: If we have something that is not loadable
if (eltTL->isLoadable()) {
// The boxed value may be shared, so we need to load the value at +0
// to make sure that we copy if we try to use it outside of the switch
// statement itself.
boxedValue = SGF.B.createLoadBorrow(loc, boxedValue);
eltCMV = {boxedValue, CastConsumptionKind::BorrowAlways};
} else {
Expand Down Expand Up @@ -2151,37 +2146,20 @@ void PatternMatchEmission::emitEnumElementDispatch(

// If the payload is boxed, project it.
if (elt->isIndirect() || elt->getParentEnum()->isIndirect()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Notice how here we check for getParentEnum().isIndirect. That is the case to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gottesmm that is the test case that I added.

SILValue boxedValue = SGF.B.createProjectBox(loc, origCMV.getValue(), 0);
eltTL = &SGF.getTypeLowering(boxedValue->getType());
ManagedValue boxedValue =
SGF.B.createProjectBox(loc, origCMV.getFinalManagedValue(), 0);
eltTL = &SGF.getTypeLowering(boxedValue.getType());
if (eltTL->isLoadable()) {
UnenforcedAccess access;
SILValue accessAddress =
access.beginAccess(SGF, loc, boxedValue, SILAccessKind::Read);

// If we needed to do another begin_access, we need to perform a
// load_copy. This is because we are going to immediately close the
// access here. If we are already in a different access, we can
// perform a load_borrow instead here.
auto accessMV = ManagedValue::forUnmanaged(accessAddress);
ManagedValue newLoadedBoxValue;
if (accessAddress == boxedValue) {
eltCMV = {SGF.B.createLoadBorrow(loc, accessMV),
CastConsumptionKind::BorrowAlways};
} else {
// Since we made a copy, send down TakeAlways.
eltCMV = {SGF.B.createLoadCopy(loc, accessMV),
CastConsumptionKind::TakeAlways};
}
access.endAccess(SGF);
boxedValue = SGF.B.createLoadBorrow(loc, boxedValue);
eltCMV = {boxedValue, CastConsumptionKind::BorrowAlways};
} else {
// The boxed value may be shared, so we always have to copy it.
eltCMV = getManagedSubobject(SGF, boxedValue, *eltTL,
eltCMV = getManagedSubobject(SGF, boxedValue.getValue(), *eltTL,
CastConsumptionKind::CopyOnSuccess);
}
}

// Reabstract to the substituted type, if needed.

CanType substEltTy =
sourceType->getTypeOfMember(SGF.SGM.M.getSwiftModule(), elt,
elt->getArgumentInterfaceType())
Expand Down
25 changes: 25 additions & 0 deletions test/SILOptimizer/access_marker_verify.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,31 @@ enum IntEnum {
// CHECK: load [trivial] [[PROJ]] : $*Int
// CHECK-LABEL: } // end sil function '$s20access_marker_verify7IntEnumO8getValueSivg'

// Also test SILGenPattern for address-only enums with indirect loadable payloads.
enum IntTEnum<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a test case for:

indirect enum

That is missing in the code-coverage.

indirect case int(Int)
case other(T)

var getValue: Int {
switch self {
case .int(let x): return x
case .other: return 0
}
}
}
// IntTEnum.getValue.getter
// CHECK-LABEL: sil hidden [ossa] @$s20access_marker_verify8IntTEnumO8getValueSivg : $@convention(method) <T> (@in_guaranteed IntTEnum<T>) -> Int {
// CHECK: bb0(%0 : $*IntTEnum<T>):
// CHECK: switch_enum_addr %{{.*}} : $*IntTEnum<T>, case #IntTEnum.int!enumelt.1: bb1, case #IntTEnum.other!enumelt.1: bb2
// CHECK: bb1:
// CHECK: [[UTEDA:%.*]] = unchecked_take_enum_data_addr %{{.*}} : $*IntTEnum<T>, #IntTEnum.int!enumelt.1
// CHECK-NOT: begin_access
// CHECK: [[BOX:%.*]] = load [take] [[UTEDA]] : $*<τ_0_0> { var Int } <T>
// CHECK: [[PROJ:%.*]] = project_box [[BOX]] : $<τ_0_0> { var Int } <T>, 0
// CHECK-NOT: begin_access
// CHECK: load [trivial] [[PROJ]] : $*Int
// CHECK-LABEL: } // end sil function '$s20access_marker_verify8IntTEnumO8getValueSivg'

// -- indirect enum reference.
enum RefEnum {
indirect case ref(BaseClass)
Expand Down