Skip to content

Commit f4caa5c

Browse files
jckarterjckarter
authored
Merge pull request #14410 from hamishknight/ignored-lvalues
[SILGen] Don't assume physical lvalue components are side effect free
2 parents ffc9405 + 0abc9f4 commit f4caa5c

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
@@ -5327,6 +5327,8 @@ class AutoreleasingWritebackComponent : public LogicalPathComponent {
53275327
AccessKind kind) const override {
53285328
return kind;
53295329
}
5330+
5331+
virtual bool isLoadingPure() const override { return true; }
53305332

53315333
void set(SILGenFunction &SGF, SILLocation loc,
53325334
ArgumentSource &&value, ManagedValue base) && override {
@@ -5663,9 +5665,10 @@ void SILGenFunction::emitIgnoredExpr(Expr *E) {
56635665
FormalEvaluationScope scope(*this);
56645666
PostponedCleanup postpone(*this);
56655667
LValue lv = emitLValue(LE->getSubExpr(), AccessKind::Read);
5666-
// If the lvalue is purely physical, then it won't have any side effects,
5667-
// and we don't need to drill into it.
5668-
if (lv.isPhysical())
5668+
5669+
// If loading from the lvalue is guaranteed to have no side effects, we
5670+
// don't need to drill into it.
5671+
if (lv.isLoadingPure())
56695672
return;
56705673

56715674
// If the last component is physical, then we just need to drill through
@@ -5681,6 +5684,39 @@ void SILGenFunction::emitIgnoredExpr(Expr *E) {
56815684
return;
56825685
}
56835686

5687+
auto findLoadThroughForceValueExprs = [](Expr *E,
5688+
SmallVectorImpl<ForceValueExpr *>
5689+
&forceValueExprs) -> LoadExpr * {
5690+
while (auto FVE = dyn_cast<ForceValueExpr>(E)) {
5691+
forceValueExprs.push_back(FVE);
5692+
E = FVE->getSubExpr();
5693+
}
5694+
return dyn_cast<LoadExpr>(E);
5695+
};
5696+
5697+
// Look through force unwrap(s) of an lvalue. If possible, we want to just to
5698+
// emit the precondition(s) without having to load the value.
5699+
SmallVector<ForceValueExpr *, 4> forceValueExprs;
5700+
if (auto *LE = findLoadThroughForceValueExprs(E, forceValueExprs)) {
5701+
FormalEvaluationScope scope(*this);
5702+
LValue lv = emitLValue(LE->getSubExpr(), AccessKind::Read);
5703+
5704+
ManagedValue value;
5705+
if (lv.isLastComponentPhysical()) {
5706+
value = emitAddressOfLValue(LE, std::move(lv), AccessKind::Read);
5707+
} else {
5708+
value = emitLoadOfLValue(LE, std::move(lv),
5709+
SGFContext::AllowImmediatePlusZero).getAsSingleValue(*this, LE);
5710+
}
5711+
5712+
for (auto &FVE : reversed(forceValueExprs)) {
5713+
const TypeLowering &optTL = getTypeLowering(FVE->getSubExpr()->getType());
5714+
value = emitCheckedGetOptionalValueFrom(
5715+
FVE, value, optTL, SGFContext::AllowImmediatePlusZero);
5716+
}
5717+
return;
5718+
}
5719+
56845720
// Otherwise, emit the result (to get any side effects), but produce it at +0
56855721
// if that allows simplification.
56865722
emitRValue(E, SGFContext::AllowImmediatePlusZero);

‎lib/SILGen/SILGenLValue.cpp

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -581,7 +581,9 @@ namespace {
581581
: PhysicalPathComponent(typeData, RefElementKind),
582582
Field(field), SubstFieldType(substFieldType),
583583
IsNonAccessing(options.IsNonAccessing) {}
584-
584+
585+
virtual bool isLoadingPure() const override { return true; }
586+
585587
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
586588
AccessKind accessKind) && override {
587589
assert(base.getType().isObject() &&
@@ -616,7 +618,9 @@ namespace {
616618
TupleElementComponent(unsigned elementIndex, LValueTypeData typeData)
617619
: PhysicalPathComponent(typeData, TupleElementKind),
618620
ElementIndex(elementIndex) {}
619-
621+
622+
virtual bool isLoadingPure() const override { return true; }
623+
620624
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
621625
AccessKind accessKind) && override {
622626
assert(base && "invalid value for element base");
@@ -640,7 +644,9 @@ namespace {
640644
LValueTypeData typeData)
641645
: PhysicalPathComponent(typeData, StructElementKind),
642646
Field(field), SubstFieldType(substFieldType) {}
643-
647+
648+
virtual bool isLoadingPure() const override { return true; }
649+
644650
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
645651
AccessKind accessKind) && override {
646652
assert(base && "invalid value for element base");
@@ -684,6 +690,8 @@ namespace {
684690
assert(getSubstFormalType() == openedArchetype);
685691
}
686692

693+
virtual bool isLoadingPure() const override { return true; }
694+
687695
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
688696
AccessKind accessKind) && override {
689697
assert(base.getType().isExistentialType() &&
@@ -732,6 +740,8 @@ namespace {
732740
: LogicalPathComponent(typeData, OpenNonOpaqueExistentialKind),
733741
OpenedArchetype(openedArchetype) {}
734742

743+
virtual bool isLoadingPure() const override { return true; }
744+
735745
AccessKind getBaseAccessKind(SILGenFunction &SGF,
736746
AccessKind kind) const override {
737747
// Always use the same access kind for the base.
@@ -832,6 +842,8 @@ namespace {
832842
assert(IsRValue || value.getType().isAddress());
833843
}
834844

845+
virtual bool isLoadingPure() const override { return true; }
846+
835847
ManagedValue offset(SILGenFunction &SGF, SILLocation loc, ManagedValue base,
836848
AccessKind accessKind) && override {
837849
assert(!base && "value component must be root of lvalue path");
@@ -1821,6 +1833,8 @@ namespace {
18211833
OrigType(origType)
18221834
{}
18231835

1836+
virtual bool isLoadingPure() const override { return true; }
1837+
18241838
RValue untranslate(SILGenFunction &SGF, SILLocation loc,
18251839
RValue &&rv, SGFContext c) && override {
18261840
return SGF.emitSubstToOrigValue(loc, std::move(rv), OrigType,
@@ -1860,6 +1874,8 @@ namespace {
18601874
SubstToOrigKind)
18611875
{}
18621876

1877+
virtual bool isLoadingPure() const override { return true; }
1878+
18631879
RValue untranslate(SILGenFunction &SGF, SILLocation loc,
18641880
RValue &&rv, SGFContext c) && override {
18651881
return SGF.emitOrigToSubstValue(loc, std::move(rv), getOrigFormalType(),
@@ -1895,6 +1911,8 @@ namespace {
18951911
: LogicalPathComponent(typeData, OwnershipKind) {
18961912
}
18971913

1914+
virtual bool isLoadingPure() const override { return true; }
1915+
18981916
AccessKind getBaseAccessKind(SILGenFunction &SGF,
18991917
AccessKind kind) const override {
19001918
// 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)