Skip to content

[SILGen] Don't assume physical lvalue components are side effect free #14410

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
Mar 1, 2018
Merged

[SILGen] Don't assume physical lvalue components are side effect free #14410

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
17 changes: 17 additions & 0 deletions lib/SILGen/LValue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,13 @@ class PathComponent {
virtual AccessKind getBaseAccessKind(SILGenFunction &SGF,
AccessKind accessKind) const = 0;

/// Is loading a value from this component guaranteed to have no observable
/// side effects?
virtual bool isLoadingPure() const {
// By default, don't assume any component is pure; components must opt-in.
return false;
}

virtual bool isRValue() const { return false; }

/// Returns the logical type-as-rvalue of the value addressed by the
Expand Down Expand Up @@ -363,6 +370,16 @@ class LValue {

bool isValid() const { return !Path.empty(); }

/// Is loading a value from this lvalue guaranteed to have no observable side
/// effects?
bool isLoadingPure() {
assert(isValid());
for (auto &component : Path)
if (!component->isLoadingPure())
return false;
return true;
}

/// Is this lvalue purely physical?
bool isPhysical() const {
assert(isValid());
Expand Down
42 changes: 39 additions & 3 deletions lib/SILGen/SILGenExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5198,6 +5198,8 @@ class AutoreleasingWritebackComponent : public LogicalPathComponent {
AccessKind kind) const override {
return kind;
}

virtual bool isLoadingPure() const override { return true; }

void set(SILGenFunction &SGF, SILLocation loc,
ArgumentSource &&value, ManagedValue base) && override {
Expand Down Expand Up @@ -5531,9 +5533,10 @@ void SILGenFunction::emitIgnoredExpr(Expr *E) {
if (auto *LE = dyn_cast<LoadExpr>(E)) {
FormalEvaluationScope scope(*this);
LValue lv = emitLValue(LE->getSubExpr(), AccessKind::Read);
// If the lvalue is purely physical, then it won't have any side effects,
// and we don't need to drill into it.
if (lv.isPhysical())

// If loading from the lvalue is guaranteed to have no side effects, we
// don't need to drill into it.
if (lv.isLoadingPure())
return;

// If the last component is physical, then we just need to drill through
Expand All @@ -5549,6 +5552,39 @@ void SILGenFunction::emitIgnoredExpr(Expr *E) {
return;
}

auto findLoadThroughForceValueExprs = [](Expr *E,
SmallVectorImpl<ForceValueExpr *>
&forceValueExprs) -> LoadExpr * {
while (auto FVE = dyn_cast<ForceValueExpr>(E)) {
forceValueExprs.push_back(FVE);
E = FVE->getSubExpr();
}
return dyn_cast<LoadExpr>(E);
};

// Look through force unwrap(s) of an lvalue. If possible, we want to just to
// emit the precondition(s) without having to load the value.
SmallVector<ForceValueExpr *, 4> forceValueExprs;
if (auto *LE = findLoadThroughForceValueExprs(E, forceValueExprs)) {
FormalEvaluationScope scope(*this);
LValue lv = emitLValue(LE->getSubExpr(), AccessKind::Read);

ManagedValue value;
if (lv.isLastComponentPhysical()) {
value = emitAddressOfLValue(LE, std::move(lv), AccessKind::Read);
} else {
value = emitLoadOfLValue(LE, std::move(lv),
SGFContext::AllowImmediatePlusZero).getAsSingleValue(*this, LE);
}

for (auto &FVE : reversed(forceValueExprs)) {
const TypeLowering &optTL = getTypeLowering(FVE->getSubExpr()->getType());
value = emitCheckedGetOptionalValueFrom(
FVE, value, optTL, SGFContext::AllowImmediatePlusZero);
}
return;
}

// Otherwise, emit the result (to get any side effects), but produce it at +0
// if that allows simplification.
emitRValue(E, SGFContext::AllowImmediatePlusZero);
Expand Down
24 changes: 21 additions & 3 deletions lib/SILGen/SILGenLValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,9 @@ namespace {
: PhysicalPathComponent(typeData, RefElementKind),
Field(field), SubstFieldType(substFieldType),
IsNonAccessing(options.IsNonAccessing) {}


virtual bool isLoadingPure() const override { return true; }

ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
AccessKind accessKind) && override {
assert(base.getType().isObject() &&
Expand Down Expand Up @@ -521,7 +523,9 @@ namespace {
TupleElementComponent(unsigned elementIndex, LValueTypeData typeData)
: PhysicalPathComponent(typeData, TupleElementKind),
ElementIndex(elementIndex) {}


virtual bool isLoadingPure() const override { return true; }

ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
AccessKind accessKind) && override {
assert(base && "invalid value for element base");
Expand All @@ -545,7 +549,9 @@ namespace {
LValueTypeData typeData)
: PhysicalPathComponent(typeData, StructElementKind),
Field(field), SubstFieldType(substFieldType) {}


virtual bool isLoadingPure() const override { return true; }

ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
AccessKind accessKind) && override {
assert(base && "invalid value for element base");
Expand Down Expand Up @@ -589,6 +595,8 @@ namespace {
assert(getSubstFormalType() == openedArchetype);
}

virtual bool isLoadingPure() const override { return true; }

ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
AccessKind accessKind) && override {
assert(base.getType().isExistentialType() &&
Expand Down Expand Up @@ -637,6 +645,8 @@ namespace {
: LogicalPathComponent(typeData, OpenNonOpaqueExistentialKind),
OpenedArchetype(openedArchetype) {}

virtual bool isLoadingPure() const override { return true; }

AccessKind getBaseAccessKind(SILGenFunction &SGF,
AccessKind kind) const override {
// Always use the same access kind for the base.
Expand Down Expand Up @@ -737,6 +747,8 @@ namespace {
assert(IsRValue || value.getType().isAddress());
}

virtual bool isLoadingPure() const override { return true; }

ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
AccessKind accessKind) && override {
assert(!base && "value component must be root of lvalue path");
Expand Down Expand Up @@ -1714,6 +1726,8 @@ namespace {
OrigType(origType)
{}

virtual bool isLoadingPure() const override { return true; }

RValue untranslate(SILGenFunction &SGF, SILLocation loc,
RValue &&rv, SGFContext c) && override {
return SGF.emitSubstToOrigValue(loc, std::move(rv), OrigType,
Expand Down Expand Up @@ -1753,6 +1767,8 @@ namespace {
SubstToOrigKind)
{}

virtual bool isLoadingPure() const override { return true; }

RValue untranslate(SILGenFunction &SGF, SILLocation loc,
RValue &&rv, SGFContext c) && override {
return SGF.emitOrigToSubstValue(loc, std::move(rv), getOrigFormalType(),
Expand Down Expand Up @@ -1788,6 +1804,8 @@ namespace {
: LogicalPathComponent(typeData, OwnershipKind) {
}

virtual bool isLoadingPure() const override { return true; }

AccessKind getBaseAccessKind(SILGenFunction &SGF,
AccessKind kind) const override {
// Always use the same access kind for the base.
Expand Down
124 changes: 121 additions & 3 deletions test/SILGen/expressions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ func testDiscardLValue() {
}


func dynamicTypePlusZero(_ a : Super1) -> Super1.Type {
func dynamicTypePlusZero(_ a: Super1) -> Super1.Type {
return type(of: a)
}
// CHECK-LABEL: dynamicTypePlusZero
Expand All @@ -567,9 +567,15 @@ func dynamicTypePlusZero(_ a : Super1) -> Super1.Type {
// CHECK: end_borrow [[BORROWED_ARG]] from [[ARG]]
// CHECK: destroy_value [[ARG]]

struct NonTrivialStruct { var c : Super1 }
struct NonTrivialStruct {
var c : Super1
var x: NonTrivialStruct? {
get { return nil }
set {}
}
}

func dontEmitIgnoredLoadExpr(_ a : NonTrivialStruct) -> NonTrivialStruct.Type {
func dontEmitIgnoredLoadExpr(_ a: NonTrivialStruct) -> NonTrivialStruct.Type {
return type(of: a)
}
// CHECK-LABEL: dontEmitIgnoredLoadExpr
Expand All @@ -581,6 +587,118 @@ func dontEmitIgnoredLoadExpr(_ a : NonTrivialStruct) -> NonTrivialStruct.Type {
// CHECK-NEXT: destroy_value %0
// CHECK-NEXT: return %4 : $@thin NonTrivialStruct.Type

// Test that we evaluate the force unwrap to get its side effects (a potential trap),
// but don't actually need to perform the load of its value.
func dontLoadIgnoredLValueForceUnwrap(_ a: inout NonTrivialStruct?) -> NonTrivialStruct.Type {
return type(of: a!)
}
// CHECK-LABEL: dontLoadIgnoredLValueForceUnwrap
// CHECK: bb0(%0 : @trivial $*Optional<NonTrivialStruct>):
// CHECK-NEXT: debug_value_addr %0
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
// CHECK-NEXT: switch_enum_addr [[READ]] : $*Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
// CHECK: bb1:
// CHECK: unreachable
// CHECK: bb2:
// CHECK-NEXT: unchecked_take_enum_data_addr [[READ]] : $*Optional<NonTrivialStruct>, #Optional.some!enumelt.1
// CHECK-NEXT: end_access [[READ]]
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
// CHECK-NEXT: return [[METATYPE]]

func dontLoadIgnoredLValueDoubleForceUnwrap(_ a: inout NonTrivialStruct??) -> NonTrivialStruct.Type {
return type(of: a!!)
}
// CHECK-LABEL: dontLoadIgnoredLValueDoubleForceUnwrap
// CHECK: bb0(%0 : @trivial $*Optional<Optional<NonTrivialStruct>>):
// CHECK-NEXT: debug_value_addr %0
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
// CHECK-NEXT: switch_enum_addr [[READ]] : $*Optional<Optional<NonTrivialStruct>>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
// CHECK: bb1:
// CHECK: unreachable
// CHECK: bb2:
// CHECK-NEXT: [[UNWRAPPED:%[0-9]+]] = unchecked_take_enum_data_addr [[READ]] : $*Optional<Optional<NonTrivialStruct>>, #Optional.some!enumelt.1
// CHECK-NEXT: switch_enum_addr [[UNWRAPPED]] : $*Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb4, case #Optional.none!enumelt: bb3
// CHECK: bb3:
// CHECK: unreachable
// CHECK: bb4:
// CHECK-NEXT: unchecked_take_enum_data_addr [[UNWRAPPED]] : $*Optional<NonTrivialStruct>, #Optional.some!enumelt.1
// CHECK-NEXT: end_access [[READ]]
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
// CHECK-NEXT: return [[METATYPE]]

func loadIgnoredLValueForceUnwrap(_ a: inout NonTrivialStruct) -> NonTrivialStruct.Type {
return type(of: a.x!)
}
// CHECK-LABEL: loadIgnoredLValueForceUnwrap
// CHECK: bb0(%0 : @trivial $*NonTrivialStruct):
// CHECK-NEXT: debug_value_addr %0
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
// CHECK-NEXT: [[BORROW:%[0-9]+]] = load_borrow [[READ]]
// CHECK-NEXT: // function_ref NonTrivialStruct.x.getter
// CHECK-NEXT: [[GETTER:%[0-9]+]] = function_ref @$S{{[_0-9a-zA-Z]*}}vg : $@convention(method) (@guaranteed NonTrivialStruct) -> @owned Optional<NonTrivialStruct>
// CHECK-NEXT: [[X:%[0-9]+]] = apply [[GETTER]]([[BORROW]])
// CHECK-NEXT: end_borrow [[BORROW]] from [[READ]]
// CHECK-NEXT: end_access [[READ]]
// CHECK-NEXT: switch_enum [[X]] : $Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
// CHECK: bb1:
// CHECK: unreachable
// CHECK: bb2([[UNWRAPPED_X:%[0-9]+]] : @owned $NonTrivialStruct):
// CHECK-NEXT: destroy_value [[UNWRAPPED_X]]
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
// CHECK-NEXT: return [[METATYPE]]

func loadIgnoredLValueThroughForceUnwrap(_ a: inout NonTrivialStruct?) -> NonTrivialStruct.Type {
return type(of: a!.x!)
}
// CHECK-LABEL: loadIgnoredLValueThroughForceUnwrap
// CHECK: bb0(%0 : @trivial $*Optional<NonTrivialStruct>):
// CHECK-NEXT: debug_value_addr %0
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
// CHECK-NEXT: switch_enum_addr [[READ]] : $*Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
// CHECK: bb1:
// CHECK: unreachable
// CHECK: bb2:
// CHECK-NEXT: [[UNWRAPPED:%[0-9]+]] = unchecked_take_enum_data_addr [[READ]] : $*Optional<NonTrivialStruct>, #Optional.some!enumelt.1
// CHECK-NEXT: [[BORROW:%[0-9]+]] = load_borrow [[UNWRAPPED]]
// CHECK-NEXT: // function_ref NonTrivialStruct.x.getter
// CHECK-NEXT: [[GETTER:%[0-9]+]] = function_ref @$S{{[_0-9a-zA-Z]*}}vg : $@convention(method) (@guaranteed NonTrivialStruct) -> @owned Optional<NonTrivialStruct>
// CHECK-NEXT: [[X:%[0-9]+]] = apply [[GETTER]]([[BORROW]])
// CHECK-NEXT: end_borrow [[BORROW]] from [[UNWRAPPED]]
// CHECK-NEXT: end_access [[READ]]
// CHECK-NEXT: switch_enum [[X]] : $Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb4, case #Optional.none!enumelt: bb3
// CHECK: bb3:
// CHECK: unreachable
// CHECK: bb4([[UNWRAPPED_X:%[0-9]+]] : @owned $NonTrivialStruct):
// CHECK-NEXT: destroy_value [[UNWRAPPED_X]]
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
// CHECK-NEXT: return [[METATYPE]]

func evaluateIgnoredKeyPathExpr(_ s: inout NonTrivialStruct, _ kp: WritableKeyPath<NonTrivialStruct, Int>) -> Int.Type {
return type(of: s[keyPath: kp])
}
// CHECK-LABEL: evaluateIgnoredKeyPathExpr
// CHECK: bb0(%0 : @trivial $*NonTrivialStruct, %1 : @owned $WritableKeyPath<NonTrivialStruct, Int>):
// CHECK-NEXT: debug_value_addr %0
// CHECK-NEXT: debug_value %1
// CHECK-NEXT: [[S_READ:%[0-9]+]] = begin_access [read] [unknown] %0
// CHECK-NEXT: [[S_TEMP:%[0-9]+]] = alloc_stack $NonTrivialStruct
// CHECK-NEXT: copy_addr [[S_READ]] to [initialization] [[S_TEMP]]
// CHECK-NEXT: [[KP_BORROW:%[0-9]+]] = begin_borrow %1
// CHECK-NEXT: [[KP_TEMP:%[0-9]+]] = copy_value [[KP_BORROW]]
// CHECK-NEXT: [[KP:%[0-9]+]] = upcast [[KP_TEMP]]
// CHECK-NEXT: [[RESULT:%[0-9]+]] = alloc_stack $Int
// CHECK-NEXT: // function_ref
// CHECK-NEXT: [[PROJECT_FN:%[0-9]+]] = function_ref @$Ss23_projectKeyPathReadOnly{{[_0-9a-zA-Z]*}}F
// CHECK-NEXT: apply [[PROJECT_FN]]<NonTrivialStruct, Int>([[RESULT]], [[S_TEMP]], [[KP]])
// CHECK-NEXT: end_access [[S_READ]]
// CHECK-NEXT: dealloc_stack [[RESULT]]
// CHECK-NEXT: end_borrow [[KP_BORROW]] from %1
// CHECK-NEXT: dealloc_stack [[S_TEMP]]
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin Int.Type
// CHECK-NEXT: destroy_value %1
// CHECK-NEXT: return [[METATYPE]]



// <rdar://problem/18851497> Swiftc fails to compile nested destructuring tuple binding
// CHECK-LABEL: sil hidden @$S11expressions21implodeRecursiveTupleyySi_Sit_SitSgF
Expand Down