Skip to content

Commit 0abc9f4

Browse files
hamishknighthamishknight
committed
[SILGen] Don't assume physical lvalue components are side effect free
When emitting an ignored expression, we try not perform a load of an lvalue if we can prove that loading has no observable side effects. Previously we based this on whether the components of the lvalue are physical, however some physical components (such as force unwrapping and key-paths) have side effects. This commit introduces a new method to determine whether a component has side effects, and also adds an additional case to `emitIgnoredExpr` to avoid loading in cases where we have a force unwrap of an lvalue load (instead, if possible, try to emit the precondition using the lvalue address).
1 parent e43ff71 commit 0abc9f4

File tree

4 files changed

+198
-9
lines changed

4 files changed

+198
-9
lines changed

‎lib/SILGen/LValue.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,13 @@ class PathComponent {
166166
virtual AccessKind getBaseAccessKind(SILGenFunction &SGF,
167167
AccessKind accessKind) const = 0;
168168

169+
/// Is loading a value from this component guaranteed to have no observable
170+
/// side effects?
171+
virtual bool isLoadingPure() const {
172+
// By default, don't assume any component is pure; components must opt-in.
173+
return false;
174+
}
175+
169176
virtual bool isRValue() const { return false; }
170177

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

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

373+
/// Is loading a value from this lvalue guaranteed to have no observable side
374+
/// effects?
375+
bool isLoadingPure() {
376+
assert(isValid());
377+
for (auto &component : Path)
378+
if (!component->isLoadingPure())
379+
return false;
380+
return true;
381+
}
382+
366383
/// Is this lvalue purely physical?
367384
bool isPhysical() const {
368385
assert(isValid());

‎lib/SILGen/SILGenExpr.cpp

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5198,6 +5198,8 @@ class AutoreleasingWritebackComponent : public LogicalPathComponent {
51985198
AccessKind kind) const override {
51995199
return kind;
52005200
}
5201+
5202+
virtual bool isLoadingPure() const override { return true; }
52015203

52025204
void set(SILGenFunction &SGF, SILLocation loc,
52035205
ArgumentSource &&value, ManagedValue base) && override {
@@ -5531,9 +5533,10 @@ void SILGenFunction::emitIgnoredExpr(Expr *E) {
55315533
if (auto *LE = dyn_cast<LoadExpr>(E)) {
55325534
FormalEvaluationScope scope(*this);
55335535
LValue lv = emitLValue(LE->getSubExpr(), AccessKind::Read);
5534-
// If the lvalue is purely physical, then it won't have any side effects,
5535-
// and we don't need to drill into it.
5536-
if (lv.isPhysical())
5536+
5537+
// If loading from the lvalue is guaranteed to have no side effects, we
5538+
// don't need to drill into it.
5539+
if (lv.isLoadingPure())
55375540
return;
55385541

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

5555+
auto findLoadThroughForceValueExprs = [](Expr *E,
5556+
SmallVectorImpl<ForceValueExpr *>
5557+
&forceValueExprs) -> LoadExpr * {
5558+
while (auto FVE = dyn_cast<ForceValueExpr>(E)) {
5559+
forceValueExprs.push_back(FVE);
5560+
E = FVE->getSubExpr();
5561+
}
5562+
return dyn_cast<LoadExpr>(E);
5563+
};
5564+
5565+
// Look through force unwrap(s) of an lvalue. If possible, we want to just to
5566+
// emit the precondition(s) without having to load the value.
5567+
SmallVector<ForceValueExpr *, 4> forceValueExprs;
5568+
if (auto *LE = findLoadThroughForceValueExprs(E, forceValueExprs)) {
5569+
FormalEvaluationScope scope(*this);
5570+
LValue lv = emitLValue(LE->getSubExpr(), AccessKind::Read);
5571+
5572+
ManagedValue value;
5573+
if (lv.isLastComponentPhysical()) {
5574+
value = emitAddressOfLValue(LE, std::move(lv), AccessKind::Read);
5575+
} else {
5576+
value = emitLoadOfLValue(LE, std::move(lv),
5577+
SGFContext::AllowImmediatePlusZero).getAsSingleValue(*this, LE);
5578+
}
5579+
5580+
for (auto &FVE : reversed(forceValueExprs)) {
5581+
const TypeLowering &optTL = getTypeLowering(FVE->getSubExpr()->getType());
5582+
value = emitCheckedGetOptionalValueFrom(
5583+
FVE, value, optTL, SGFContext::AllowImmediatePlusZero);
5584+
}
5585+
return;
5586+
}
5587+
55525588
// Otherwise, emit the result (to get any side effects), but produce it at +0
55535589
// if that allows simplification.
55545590
emitRValue(E, SGFContext::AllowImmediatePlusZero);

‎lib/SILGen/SILGenLValue.cpp

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -486,7 +486,9 @@ namespace {
486486
: PhysicalPathComponent(typeData, RefElementKind),
487487
Field(field), SubstFieldType(substFieldType),
488488
IsNonAccessing(options.IsNonAccessing) {}
489-
489+
490+
virtual bool isLoadingPure() const override { return true; }
491+
490492
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
491493
AccessKind accessKind) && override {
492494
assert(base.getType().isObject() &&
@@ -521,7 +523,9 @@ namespace {
521523
TupleElementComponent(unsigned elementIndex, LValueTypeData typeData)
522524
: PhysicalPathComponent(typeData, TupleElementKind),
523525
ElementIndex(elementIndex) {}
524-
526+
527+
virtual bool isLoadingPure() const override { return true; }
528+
525529
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
526530
AccessKind accessKind) && override {
527531
assert(base && "invalid value for element base");
@@ -545,7 +549,9 @@ namespace {
545549
LValueTypeData typeData)
546550
: PhysicalPathComponent(typeData, StructElementKind),
547551
Field(field), SubstFieldType(substFieldType) {}
548-
552+
553+
virtual bool isLoadingPure() const override { return true; }
554+
549555
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
550556
AccessKind accessKind) && override {
551557
assert(base && "invalid value for element base");
@@ -589,6 +595,8 @@ namespace {
589595
assert(getSubstFormalType() == openedArchetype);
590596
}
591597

598+
virtual bool isLoadingPure() const override { return true; }
599+
592600
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
593601
AccessKind accessKind) && override {
594602
assert(base.getType().isExistentialType() &&
@@ -637,6 +645,8 @@ namespace {
637645
: LogicalPathComponent(typeData, OpenNonOpaqueExistentialKind),
638646
OpenedArchetype(openedArchetype) {}
639647

648+
virtual bool isLoadingPure() const override { return true; }
649+
640650
AccessKind getBaseAccessKind(SILGenFunction &SGF,
641651
AccessKind kind) const override {
642652
// Always use the same access kind for the base.
@@ -737,6 +747,8 @@ namespace {
737747
assert(IsRValue || value.getType().isAddress());
738748
}
739749

750+
virtual bool isLoadingPure() const override { return true; }
751+
740752
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
741753
AccessKind accessKind) && override {
742754
assert(!base && "value component must be root of lvalue path");
@@ -1714,6 +1726,8 @@ namespace {
17141726
OrigType(origType)
17151727
{}
17161728

1729+
virtual bool isLoadingPure() const override { return true; }
1730+
17171731
RValue untranslate(SILGenFunction &SGF, SILLocation loc,
17181732
RValue &&rv, SGFContext c) && override {
17191733
return SGF.emitSubstToOrigValue(loc, std::move(rv), OrigType,
@@ -1753,6 +1767,8 @@ namespace {
17531767
SubstToOrigKind)
17541768
{}
17551769

1770+
virtual bool isLoadingPure() const override { return true; }
1771+
17561772
RValue untranslate(SILGenFunction &SGF, SILLocation loc,
17571773
RValue &&rv, SGFContext c) && override {
17581774
return SGF.emitOrigToSubstValue(loc, std::move(rv), getOrigFormalType(),
@@ -1788,6 +1804,8 @@ namespace {
17881804
: LogicalPathComponent(typeData, OwnershipKind) {
17891805
}
17901806

1807+
virtual bool isLoadingPure() const override { return true; }
1808+
17911809
AccessKind getBaseAccessKind(SILGenFunction &SGF,
17921810
AccessKind kind) const override {
17931811
// Always use the same access kind for the base.

‎test/SILGen/expressions.swift

Lines changed: 121 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,7 @@ func testDiscardLValue() {
555555
}
556556

557557

558-
func dynamicTypePlusZero(_ a : Super1) -> Super1.Type {
558+
func dynamicTypePlusZero(_ a: Super1) -> Super1.Type {
559559
return type(of: a)
560560
}
561561
// CHECK-LABEL: dynamicTypePlusZero
@@ -567,9 +567,15 @@ func dynamicTypePlusZero(_ a : Super1) -> Super1.Type {
567567
// CHECK: end_borrow [[BORROWED_ARG]] from [[ARG]]
568568
// CHECK: destroy_value [[ARG]]
569569

570-
struct NonTrivialStruct { var c : Super1 }
570+
struct NonTrivialStruct {
571+
var c : Super1
572+
var x: NonTrivialStruct? {
573+
get { return nil }
574+
set {}
575+
}
576+
}
571577

572-
func dontEmitIgnoredLoadExpr(_ a : NonTrivialStruct) -> NonTrivialStruct.Type {
578+
func dontEmitIgnoredLoadExpr(_ a: NonTrivialStruct) -> NonTrivialStruct.Type {
573579
return type(of: a)
574580
}
575581
// CHECK-LABEL: dontEmitIgnoredLoadExpr
@@ -581,6 +587,118 @@ func dontEmitIgnoredLoadExpr(_ a : NonTrivialStruct) -> NonTrivialStruct.Type {
581587
// CHECK-NEXT: destroy_value %0
582588
// CHECK-NEXT: return %4 : $@thin NonTrivialStruct.Type
583589

590+
// Test that we evaluate the force unwrap to get its side effects (a potential trap),
591+
// but don't actually need to perform the load of its value.
592+
func dontLoadIgnoredLValueForceUnwrap(_ a: inout NonTrivialStruct?) -> NonTrivialStruct.Type {
593+
return type(of: a!)
594+
}
595+
// CHECK-LABEL: dontLoadIgnoredLValueForceUnwrap
596+
// CHECK: bb0(%0 : @trivial $*Optional<NonTrivialStruct>):
597+
// CHECK-NEXT: debug_value_addr %0
598+
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
599+
// CHECK-NEXT: switch_enum_addr [[READ]] : $*Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
600+
// CHECK: bb1:
601+
// CHECK: unreachable
602+
// CHECK: bb2:
603+
// CHECK-NEXT: unchecked_take_enum_data_addr [[READ]] : $*Optional<NonTrivialStruct>, #Optional.some!enumelt.1
604+
// CHECK-NEXT: end_access [[READ]]
605+
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
606+
// CHECK-NEXT: return [[METATYPE]]
607+
608+
func dontLoadIgnoredLValueDoubleForceUnwrap(_ a: inout NonTrivialStruct??) -> NonTrivialStruct.Type {
609+
return type(of: a!!)
610+
}
611+
// CHECK-LABEL: dontLoadIgnoredLValueDoubleForceUnwrap
612+
// CHECK: bb0(%0 : @trivial $*Optional<Optional<NonTrivialStruct>>):
613+
// CHECK-NEXT: debug_value_addr %0
614+
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
615+
// CHECK-NEXT: switch_enum_addr [[READ]] : $*Optional<Optional<NonTrivialStruct>>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
616+
// CHECK: bb1:
617+
// CHECK: unreachable
618+
// CHECK: bb2:
619+
// CHECK-NEXT: [[UNWRAPPED:%[0-9]+]] = unchecked_take_enum_data_addr [[READ]] : $*Optional<Optional<NonTrivialStruct>>, #Optional.some!enumelt.1
620+
// CHECK-NEXT: switch_enum_addr [[UNWRAPPED]] : $*Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb4, case #Optional.none!enumelt: bb3
621+
// CHECK: bb3:
622+
// CHECK: unreachable
623+
// CHECK: bb4:
624+
// CHECK-NEXT: unchecked_take_enum_data_addr [[UNWRAPPED]] : $*Optional<NonTrivialStruct>, #Optional.some!enumelt.1
625+
// CHECK-NEXT: end_access [[READ]]
626+
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
627+
// CHECK-NEXT: return [[METATYPE]]
628+
629+
func loadIgnoredLValueForceUnwrap(_ a: inout NonTrivialStruct) -> NonTrivialStruct.Type {
630+
return type(of: a.x!)
631+
}
632+
// CHECK-LABEL: loadIgnoredLValueForceUnwrap
633+
// CHECK: bb0(%0 : @trivial $*NonTrivialStruct):
634+
// CHECK-NEXT: debug_value_addr %0
635+
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
636+
// CHECK-NEXT: [[BORROW:%[0-9]+]] = load_borrow [[READ]]
637+
// CHECK-NEXT: // function_ref NonTrivialStruct.x.getter
638+
// CHECK-NEXT: [[GETTER:%[0-9]+]] = function_ref @$S{{[_0-9a-zA-Z]*}}vg : $@convention(method) (@guaranteed NonTrivialStruct) -> @owned Optional<NonTrivialStruct>
639+
// CHECK-NEXT: [[X:%[0-9]+]] = apply [[GETTER]]([[BORROW]])
640+
// CHECK-NEXT: end_borrow [[BORROW]] from [[READ]]
641+
// CHECK-NEXT: end_access [[READ]]
642+
// CHECK-NEXT: switch_enum [[X]] : $Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
643+
// CHECK: bb1:
644+
// CHECK: unreachable
645+
// CHECK: bb2([[UNWRAPPED_X:%[0-9]+]] : @owned $NonTrivialStruct):
646+
// CHECK-NEXT: destroy_value [[UNWRAPPED_X]]
647+
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
648+
// CHECK-NEXT: return [[METATYPE]]
649+
650+
func loadIgnoredLValueThroughForceUnwrap(_ a: inout NonTrivialStruct?) -> NonTrivialStruct.Type {
651+
return type(of: a!.x!)
652+
}
653+
// CHECK-LABEL: loadIgnoredLValueThroughForceUnwrap
654+
// CHECK: bb0(%0 : @trivial $*Optional<NonTrivialStruct>):
655+
// CHECK-NEXT: debug_value_addr %0
656+
// CHECK-NEXT: [[READ:%[0-9]+]] = begin_access [read] [unknown] %0
657+
// CHECK-NEXT: switch_enum_addr [[READ]] : $*Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1
658+
// CHECK: bb1:
659+
// CHECK: unreachable
660+
// CHECK: bb2:
661+
// CHECK-NEXT: [[UNWRAPPED:%[0-9]+]] = unchecked_take_enum_data_addr [[READ]] : $*Optional<NonTrivialStruct>, #Optional.some!enumelt.1
662+
// CHECK-NEXT: [[BORROW:%[0-9]+]] = load_borrow [[UNWRAPPED]]
663+
// CHECK-NEXT: // function_ref NonTrivialStruct.x.getter
664+
// CHECK-NEXT: [[GETTER:%[0-9]+]] = function_ref @$S{{[_0-9a-zA-Z]*}}vg : $@convention(method) (@guaranteed NonTrivialStruct) -> @owned Optional<NonTrivialStruct>
665+
// CHECK-NEXT: [[X:%[0-9]+]] = apply [[GETTER]]([[BORROW]])
666+
// CHECK-NEXT: end_borrow [[BORROW]] from [[UNWRAPPED]]
667+
// CHECK-NEXT: end_access [[READ]]
668+
// CHECK-NEXT: switch_enum [[X]] : $Optional<NonTrivialStruct>, case #Optional.some!enumelt.1: bb4, case #Optional.none!enumelt: bb3
669+
// CHECK: bb3:
670+
// CHECK: unreachable
671+
// CHECK: bb4([[UNWRAPPED_X:%[0-9]+]] : @owned $NonTrivialStruct):
672+
// CHECK-NEXT: destroy_value [[UNWRAPPED_X]]
673+
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin NonTrivialStruct.Type
674+
// CHECK-NEXT: return [[METATYPE]]
675+
676+
func evaluateIgnoredKeyPathExpr(_ s: inout NonTrivialStruct, _ kp: WritableKeyPath<NonTrivialStruct, Int>) -> Int.Type {
677+
return type(of: s[keyPath: kp])
678+
}
679+
// CHECK-LABEL: evaluateIgnoredKeyPathExpr
680+
// CHECK: bb0(%0 : @trivial $*NonTrivialStruct, %1 : @owned $WritableKeyPath<NonTrivialStruct, Int>):
681+
// CHECK-NEXT: debug_value_addr %0
682+
// CHECK-NEXT: debug_value %1
683+
// CHECK-NEXT: [[S_READ:%[0-9]+]] = begin_access [read] [unknown] %0
684+
// CHECK-NEXT: [[S_TEMP:%[0-9]+]] = alloc_stack $NonTrivialStruct
685+
// CHECK-NEXT: copy_addr [[S_READ]] to [initialization] [[S_TEMP]]
686+
// CHECK-NEXT: [[KP_BORROW:%[0-9]+]] = begin_borrow %1
687+
// CHECK-NEXT: [[KP_TEMP:%[0-9]+]] = copy_value [[KP_BORROW]]
688+
// CHECK-NEXT: [[KP:%[0-9]+]] = upcast [[KP_TEMP]]
689+
// CHECK-NEXT: [[RESULT:%[0-9]+]] = alloc_stack $Int
690+
// CHECK-NEXT: // function_ref
691+
// CHECK-NEXT: [[PROJECT_FN:%[0-9]+]] = function_ref @$Ss23_projectKeyPathReadOnly{{[_0-9a-zA-Z]*}}F
692+
// CHECK-NEXT: apply [[PROJECT_FN]]<NonTrivialStruct, Int>([[RESULT]], [[S_TEMP]], [[KP]])
693+
// CHECK-NEXT: end_access [[S_READ]]
694+
// CHECK-NEXT: dealloc_stack [[RESULT]]
695+
// CHECK-NEXT: end_borrow [[KP_BORROW]] from %1
696+
// CHECK-NEXT: dealloc_stack [[S_TEMP]]
697+
// CHECK-NEXT: [[METATYPE:%[0-9]+]] = metatype $@thin Int.Type
698+
// CHECK-NEXT: destroy_value %1
699+
// CHECK-NEXT: return [[METATYPE]]
700+
701+
584702

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

0 commit comments

Comments
 (0)