Skip to content

SILGen: Emit move-only bases for lvalues in a borrow scope. #65302

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.

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
Apr 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions lib/SILGen/LValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ class PathComponent {
CoroutineAccessorKind, // coroutine accessor
ValueKind, // random base pointer as an lvalue
PhysicalKeyPathApplicationKind, // applying a key path
BorrowValueKind, // load_borrow the base rvalue for a projection

// Logical LValue kinds
GetterSetterKind, // property or subscript getter/setter
Expand All @@ -114,6 +115,7 @@ class PathComponent {
WritebackPseudoKind, // a fake component to customize writeback
OpenNonOpaqueExistentialKind, // opened class or metatype existential
LogicalKeyPathApplicationKind, // applying a key path

// Translation LValue kinds (a subtype of logical)
OrigToSubstKind, // generic type substitution
SubstToOrigKind, // generic type substitution
Expand Down
42 changes: 40 additions & 2 deletions lib/SILGen/SILGenLValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,28 @@ namespace {
Value.dump(OS, indent + 2);
}
};

class BorrowValueComponent : public PhysicalPathComponent {
public:
BorrowValueComponent(LValueTypeData typeData)
: PhysicalPathComponent(typeData, BorrowValueKind, None) {}

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

ManagedValue project(SILGenFunction &SGF, SILLocation loc,
ManagedValue base) && override {
assert(base
&& base.getType().isAddress()
&& "should have an address base to borrow from");
auto result = SGF.B.createLoadBorrow(loc, base.getValue());
return SGF.emitFormalEvaluationManagedBorrowedRValueWithCleanup(loc,
base.getValue(), result);
}

void dump(raw_ostream &OS, unsigned indent) const override {
OS.indent(indent) << "BorrowValueComponent\n";
}
};
} // end anonymous namespace

static bool isReadNoneFunction(const Expr *e) {
Expand Down Expand Up @@ -2760,7 +2782,7 @@ static ManagedValue visitRecNonInOutBase(SILGenLValue &SGL, Expr *e,
}
}
}

if (SGF.SGM.Types.isIndirectPlusZeroSelfParameter(e->getType())) {
ctx = SGFContext::AllowGuaranteedPlusZero;
}
Expand Down Expand Up @@ -2788,6 +2810,22 @@ LValue SILGenLValue::visitRec(Expr *e, SGFAccessKind accessKind,
if (e->getType()->is<LValueType>() || e->isSemanticallyInOutExpr()) {
return visitRecInOut(*this, e, accessKind, options, orig);
}

// If the base is a load of a noncopyable type (or, eventually, when we have
// a `borrow x` operator, the operator is used on the base here), we want to
// apply the lvalue within a formal access to the original value instead of
// an actual loaded copy.
Comment on lines +2814 to +2817
Copy link
Member

@kavon kavon Apr 20, 2023

Choose a reason for hiding this comment

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

So we do have the expression form _borrow x behind -enable-experimental-move-only, so it's worth checking if this also works for that too. This:

struct X: ~Copyable {
  var something: Int { return 1 }
}

func f() -> Int {
  var x = X()
  return (_borrow x).something
}

gives an AST like this:

(member_ref_expr ...
  (paren_expr ...
    (load_expr implicit type='X' ...
      (borrow_expr type='@lvalue X' ...
        (declref_expr type='@lvalue X' ... )))))

It doesn't appear that SILGenLValue has a visitor for a BorrowExpr yet, so this might crash now (haven' tried your branch yet).

if (e->getType()->isPureMoveOnly()) {
if (auto load = dyn_cast<LoadExpr>(e)) {
LValue lv = visitRec(load->getSubExpr(), SGFAccessKind::BorrowedAddressRead,
options, orig);
CanType formalType = getSubstFormalRValueType(e);
LValueTypeData typeData{accessKind, AbstractionPattern(formalType),
formalType, lv.getTypeOfRValue().getASTType()};
lv.add<BorrowValueComponent>(typeData);
return lv;
}
}

// Otherwise we have a non-lvalue type (references, values, metatypes,
// etc). These act as the root of a logical lvalue. Compute the root value,
Expand Down Expand Up @@ -3493,7 +3531,7 @@ LValue SILGenLValue::visitMemberRefExpr(MemberRefExpr *e,
SGF.F.getResilienceExpansion());
}
}

LValue lv = visitRec(e->getBase(),
getBaseAccessKind(SGF.SGM, var, accessKind, strategy,
getBaseFormalType(e->getBase())),
Expand Down
30 changes: 30 additions & 0 deletions test/Interpreter/moveonly_computed_property_in_class.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// RUN: %target-run-simple-swift
// REQUIRES: executable_test
@_moveOnly
struct FileDescriptor {
let desc: Int

var empty: Bool { return desc == Int.min }
func isEmpty() -> Bool { return empty }
}

final class Wrapper {
var val: FileDescriptor = FileDescriptor(desc: 0)

func isEmpty_bug() -> Bool {
// error: 'self.val' has consuming use that cannot
// be eliminated due to a tight exclusivity scope
return val.empty // note: consuming use here
Comment on lines +15 to +17
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can remove these comments now that it's working

}

func isEmpty_ok() -> Bool {
return val.isEmpty()
}
}

let w = Wrapper()
// CHECK: is not empty
print(w.isEmpty_bug() ? "is empty" : "is not empty")
w.val = FileDescriptor(desc: Int.min)
// CHECK: is empty
print(w.isEmpty_bug() ? "is empty" : "is not empty")
47 changes: 15 additions & 32 deletions test/SILOptimizer/moveonly_addresschecker_diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -377,47 +377,38 @@ public func classAssignToVar5Arg2(_ x: __shared Klass, _ x2: inout Klass) { // e

public func classAccessAccessField(_ x: __shared Klass) { // expected-error {{'x' has guaranteed ownership but was consumed}}
var x2 = x // expected-note {{consuming use here}}
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
x2 = Klass()
borrowVal(x2.k) // expected-note {{consuming use here}}
borrowVal(x2.k)
for _ in 0..<1024 {
borrowVal(x2.k) // expected-note {{consuming use here}}
borrowVal(x2.k)
}
}

public func classAccessAccessFieldArg(_ x2: inout Klass) {
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
borrowVal(x2.k) // expected-note {{consuming use here}}
for _ in 0..<1024 {
borrowVal(x2.k) // expected-note {{consuming use here}}
borrowVal(x2.k)
}
}

public func classAccessConsumeField(_ x: __shared Klass) { // expected-error {{'x' has guaranteed ownership but was consumed}}
var x2 = x // expected-note {{consuming use here}}
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
x2 = Klass()
// Since a class is a reference type, we do not emit an error here.
consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}
for _ in 0..<1024 {
consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}
}
}

public func classAccessConsumeFieldArg(_ x2: inout Klass) {
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// Since a class is a reference type, we do not emit an error here.
consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}

for _ in 0..<1024 {
consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}
}
}
Expand Down Expand Up @@ -682,45 +673,37 @@ public func finalClassAssignToVar5Arg2(_ x2: inout FinalKlass) {

public func finalClassAccessField() {
var x2 = FinalKlass()
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
x2 = FinalKlass()
borrowVal(x2.k) // expected-note {{consuming use here}}
borrowVal(x2.k)
for _ in 0..<1024 {
borrowVal(x2.k) // expected-note {{consuming use here}}
borrowVal(x2.k)
}
}

public func finalClassAccessFieldArg(_ x2: inout FinalKlass) {
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
borrowVal(x2.k) // expected-note {{consuming use here}}
borrowVal(x2.k)
for _ in 0..<1024 {
borrowVal(x2.k) // expected-note {{consuming use here}}
borrowVal(x2.k)
}
}

public func finalClassConsumeField() {
var x2 = FinalKlass()
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
x2 = FinalKlass()

consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}
for _ in 0..<1024 {
consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}
}
}

public func finalClassConsumeFieldArg(_ x2: inout FinalKlass) {
// expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
// expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}
for _ in 0..<1024 {
consumeVal(x2.k) // expected-note {{consuming use here}}
consumeVal(x2.k)
// expected-error @-1 {{'x2.k' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it}}
}
}
Expand Down
21 changes: 21 additions & 0 deletions test/SILOptimizer/moveonly_computed_property.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %target-swift-frontend -emit-sil -verify %s

// Applying a computed property to a move-only field in a class should occur
// entirely within a formal access to the class property. rdar://105794506

@_moveOnly
struct FileDescriptor {
private let desc: Int

var empty: Bool { return desc == Int.min }
}

final class Wrapper {
private var val: FileDescriptor

func isEmpty_bug() -> Bool {
return val.empty
}

init() { fatalError() }
}