Skip to content

const evaluator: enums #21869

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 2 commits into from
Jan 22, 2019
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
39 changes: 39 additions & 0 deletions include/swift/SIL/SILConstants.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@ class SymbolicValue {
/// "aggregate" member of the value union.
RK_Aggregate,

/// This value is an enum with no payload.
RK_Enum,

/// This value is an enum with a payload.
RK_EnumWithPayload,

/// This represents the address of a memory object.
RK_DirectAddress,

Expand Down Expand Up @@ -136,6 +142,14 @@ class SymbolicValue {
/// information about the array elements and count.
const SymbolicValue *aggregate;

/// When this SymbolicValue is of "Enum" kind, this pointer stores
/// information about the enum case type.
EnumElementDecl *enumVal;

/// When this SymbolicValue is of "EnumWithPayload" kind, this pointer
/// stores information about the enum case type and its payload.
EnumWithPayloadSymbolicValue *enumValWithPayload;

/// When the representationKind is "DirectAddress", this pointer is the
/// memory object referenced.
SymbolicValueMemoryObject *directAddress;
Expand Down Expand Up @@ -186,6 +200,12 @@ class SymbolicValue {
/// This can be an array, struct, tuple, etc.
Aggregate,

/// This is an enum without payload.
Enum,

/// This is an enum with payload (formally known as "associated value").
EnumWithPayload,

/// This value represents the address of, or into, a memory object.
Address,

Expand Down Expand Up @@ -271,6 +291,25 @@ class SymbolicValue {

ArrayRef<SymbolicValue> getAggregateValue() const;

/// This returns a constant Symbolic value for the enum case in `decl`, which
/// must not have an associated value.
static SymbolicValue getEnum(EnumElementDecl *decl) {
assert(decl);
SymbolicValue result;
result.representationKind = RK_Enum;
result.value.enumVal = decl;
return result;
}

/// `payload` must be a constant.
static SymbolicValue getEnumWithPayload(EnumElementDecl *decl,
SymbolicValue payload,
ASTContext &astContext);

EnumElementDecl *getEnumValue() const;

SymbolicValue getEnumPayloadValue() const;

/// Return a symbolic value that represents the address of a memory object.
static SymbolicValue getAddress(SymbolicValueMemoryObject *memoryObject) {
SymbolicValue result;
Expand Down
73 changes: 73 additions & 0 deletions lib/SIL/SILConstants.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ void SymbolicValue::print(llvm::raw_ostream &os, unsigned indent) const {
return;
}
}
case RK_Enum: {
auto *decl = getEnumValue();
os << "enum: ";
decl->print(os);
return;
}
case RK_EnumWithPayload: {
auto *decl = getEnumValue();
os << "enum: ";
decl->print(os);
os << ", payload: ";
getEnumPayloadValue().print(os, indent);
return;
}
case RK_DirectAddress:
case RK_DerivedAddress: {
SmallVector<unsigned, 4> accessPath;
Expand Down Expand Up @@ -111,6 +125,10 @@ SymbolicValue::Kind SymbolicValue::getKind() const {
return Function;
case RK_Aggregate:
return Aggregate;
case RK_Enum:
return Enum;
case RK_EnumWithPayload:
return EnumWithPayload;
case RK_Integer:
case RK_IntegerInline:
return Integer;
Expand All @@ -133,6 +151,9 @@ SymbolicValue::cloneInto(ASTContext &astContext) const {
case RK_Metatype:
case RK_Function:
assert(0 && "cloning this representation kind is not supported");
case RK_Enum:
// These have trivial inline storage, just return a copy.
return *this;
case RK_IntegerInline:
case RK_Integer:
return SymbolicValue::getInteger(getIntegerValue(), astContext);
Expand All @@ -146,6 +167,8 @@ SymbolicValue::cloneInto(ASTContext &astContext) const {
results.push_back(elt.cloneInto(astContext));
return getAggregate(results, astContext);
}
case RK_EnumWithPayload:
return getEnumWithPayload(getEnumValue(), getEnumPayloadValue(), astContext);
case RK_DirectAddress:
case RK_DerivedAddress: {
SmallVector<unsigned, 4> accessPath;
Expand Down Expand Up @@ -354,6 +377,56 @@ UnknownReason SymbolicValue::getUnknownReason() const {
return value.unknown->reason;
}

//===----------------------------------------------------------------------===//
// Enums
//===----------------------------------------------------------------------===//

namespace swift {

/// This is the representation of a constant enum value with payload.
struct EnumWithPayloadSymbolicValue final {
/// The enum case.
EnumElementDecl *enumDecl;
SymbolicValue payload;

EnumWithPayloadSymbolicValue(EnumElementDecl *decl, SymbolicValue payload)
: enumDecl(decl), payload(payload) {}

private:
EnumWithPayloadSymbolicValue() = delete;
EnumWithPayloadSymbolicValue(const EnumWithPayloadSymbolicValue &) = delete;
};
} // end namespace swift

/// This returns a constant Symbolic value for the enum case in `decl` with a
/// payload.
SymbolicValue
SymbolicValue::getEnumWithPayload(EnumElementDecl *decl, SymbolicValue payload,
ASTContext &astContext) {
assert(decl && payload.isConstant());
auto rawMem = astContext.Allocate(sizeof(EnumWithPayloadSymbolicValue),
alignof(EnumWithPayloadSymbolicValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

@marcrasi I have a more general question. It would be helpful if you can throw some light on why we have to allocate the symbolic values in the allocator of the astContext, and not bind its lifetime to the interpreter instance i.e, ConstExprEvaluator instance. (I think you clarified this in the earlier PRs, but it slipped my mind.) This essentially means these objects are never cleaned up (or at least retained for a long time). On the upside, I see that this enables querying the symbolic state of a SILValue long after interpreter has died. I wonder whether there is an inherent reason why this is needed or is this a design choice motivated by other use cases (possibly Swift for Tensorflow usecases).

Copy link
Author

Choose a reason for hiding this comment

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

One of our passes does store SymbolicValues into a SILInstruction that lives longer than the ConstExprEvaluator. (see GraphOperationInst in SILInstruction.h in the tensorflow branch if you're intersted, and look at the call to constantEvaluator.computeConstantValues in TFDeabstraction.cpp).

It should be pretty easy to clone all the memory into a longer-lived allocator before we do that though. It would be good to write APIs such that it's hard to forget to do this. Maybe all the public ConstExprEvaluator functions could clone all the memory for the returned SymbolicValues into the astcontext by default. There could be an argument allowing callers who don't actually need long-lived SymbolicValues to opt out of the cloning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining this.

Maybe all the public ConstExprEvaluator functions could clone all the memory for the returned SymbolicValues into the astcontext by default.

It seems to me that even cloning symbolic values returned by public functions may not necessary in many cases. I think it would be better if they are cloned only when they have to be stored in an instruction. If you are fine with this, I can later make a separate PR that introduces bump allocator in ConstExprEvaluator and stores the symbolic values in there.

Also, from my quick search through the code base it appears that SILInstructions are generally allocated using M.allocateInst, which seems to have a shorter lifetime compared to astcontext. It gets freed when the containing function is destroyed etc. That may something to consider for GraphOperationInst, if you have not already.

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative is that we can also provide a suitable allocator as a parameter to the constructor of ConstExprEvaluator (e.g. as Lambda or something else). But, this is something we can discuss in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcrasi I am thinking of moving some design discussions such as the above to Swift forum (compiler development section), so that we can discuss these things is more public visibility
and hopefully involve more people. :-)

auto enumVal = ::new (rawMem) EnumWithPayloadSymbolicValue(decl, payload);

SymbolicValue result;
result.representationKind = RK_EnumWithPayload;
result.value.enumValWithPayload = enumVal;
return result;
}

EnumElementDecl *SymbolicValue::getEnumValue() const {
if (representationKind == RK_Enum)
return value.enumVal;

assert(representationKind == RK_EnumWithPayload);
return value.enumValWithPayload->enumDecl;
}

SymbolicValue SymbolicValue::getEnumPayloadValue() const {
assert(representationKind == RK_EnumWithPayload);
return value.enumValWithPayload->payload;
}

//===----------------------------------------------------------------------===//
// Addresses
//===----------------------------------------------------------------------===//
Expand Down
47 changes: 47 additions & 0 deletions lib/SILOptimizer/Utils/ConstExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,25 @@ SymbolicValue ConstExprFunctionState::computeConstantValue(SILValue value) {
return calculatedValues[apply];
}

if (auto *enumVal = dyn_cast<EnumInst>(value)) {
if (!enumVal->hasOperand())
return SymbolicValue::getEnum(enumVal->getElement());

auto payload = getConstantValue(enumVal->getOperand());
if (!payload.isConstant())
return payload;
return SymbolicValue::getEnumWithPayload(enumVal->getElement(), payload,
evaluator.getASTContext());
}

// This one returns the address of its enum payload.
if (auto *dai = dyn_cast<UncheckedTakeEnumDataAddrInst>(value)) {
auto enumVal = getConstAddrAndLoadResult(dai->getOperand());
if (!enumVal.isConstant())
return enumVal;
return createMemoryObject(value, enumVal.getEnumPayloadValue());
}

// This instruction is a marker that returns its first operand.
if (auto *bai = dyn_cast<BeginAccessInst>(value))
return getConstantValue(bai->getOperand());
Expand Down Expand Up @@ -1244,6 +1263,34 @@ static llvm::Optional<SymbolicValue> evaluateAndCacheCall(
continue;
}

if (isa<SwitchEnumAddrInst>(inst) || isa<SwitchEnumInst>(inst)) {
SymbolicValue value;
SwitchEnumInstBase *switchInst = dyn_cast<SwitchEnumInst>(inst);
if (switchInst) {
value = state.getConstantValue(switchInst->getOperand());
} else {
switchInst = cast<SwitchEnumAddrInst>(inst);
value = state.getConstAddrAndLoadResult(switchInst->getOperand());
}
if (!value.isConstant())
return value;
assert(value.getKind() == SymbolicValue::Enum ||
value.getKind() == SymbolicValue::EnumWithPayload);
// Set up basic block arguments.
auto *caseBB = switchInst->getCaseDestination(value.getEnumValue());
if (caseBB->getNumArguments() > 0) {
assert(value.getKind() == SymbolicValue::EnumWithPayload);
// When there are multiple payload components, they form a single
// tuple-typed argument.
assert(caseBB->getNumArguments() == 1);
auto argument = value.getEnumPayloadValue();
assert(argument.isConstant());
state.setValue(caseBB->getArgument(0), argument);
}
nextInst = caseBB->begin();
continue;
}

LLVM_DEBUG(llvm::dbgs()
<< "ConstExpr: Unknown Terminator: " << *inst << "\n");

Expand Down
88 changes: 88 additions & 0 deletions test/SILOptimizer/pound_assert.swift
Original file line number Diff line number Diff line change
Expand Up @@ -497,3 +497,91 @@ func stringInitNonEmptyFlowSensitive() -> ContainsString {
func invokeStringInitNonEmptyFlowSensitive() {
#assert(stringInitNonEmptyFlowSensitive().x == 1)
}

//===----------------------------------------------------------------------===//
// Enums and optionals.
//===----------------------------------------------------------------------===//
func isNil(_ x: Int?) -> Bool {
return x == nil
}

#assert(isNil(nil))
#assert(!isNil(3))

public enum Pet {
case bird
case cat(Int)
case dog(Int, Int)
case fish
}

public func weighPet(pet: Pet) -> Int {
switch pet {
case .bird: return 3
case let .cat(weight): return weight
case let .dog(w1, w2): return w1+w2
default: return 1
}
}

#assert(weighPet(pet: .bird) == 3)
#assert(weighPet(pet: .fish) == 1)
#assert(weighPet(pet: .cat(2)) == 2)
// expected-error @+1 {{assertion failed}}
#assert(weighPet(pet: .cat(2)) == 3)
#assert(weighPet(pet: .dog(9, 10)) == 19)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether this patch handles indirect, recursive enums, or if it needs something more? I think it would be great to add a test for indirect, recursive enums. It may help to document the behavior on them.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed! Indirect enums seem not to work -- the thing that is immediately blocking them is that the const evaluator does not do alloc_box. I have added some test code but commented out the #assert statements that actually trigger the test. I'd prefer not to implement indirect enum support myself, and there's nothing left in the tensorflow branch to copy over, so if you want it, would you or someone else take that on after this gets merged?

Also your question reminded me to add a test for address-only enums. So I did that and that seems to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Thanks for adding a test. We can add the support for indirect enum when it is needed. It is enough that we document that using test cases here.


// Test indirect enums.
indirect enum IntExpr {
case int(_ value: Int)
case add(_ lhs: IntExpr, _ rhs: IntExpr)
case multiply(_ lhs: IntExpr, _ rhs: IntExpr)
}

Copy link
Contributor

@ravikandhadai ravikandhadai Jan 17, 2019

Choose a reason for hiding this comment

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

Looks like there are some extra spaces here, and also in the code below

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean the empty lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, the red bars were underscores. It looked like GitHub was flagging was extra spaces (diffs)

func evaluate(intExpr: IntExpr) -> Int {
switch intExpr {
case .int(let value):
return value
case .add(let lhs, let rhs):
return evaluate(intExpr: lhs) + evaluate(intExpr: rhs)
case .multiply(let lhs, let rhs):
return evaluate(intExpr: lhs) * evaluate(intExpr: rhs)
}
}

// TODO: The constant evaluator can't handle indirect enums yet.
// expected-error @+2 {{#assert condition not constant}}
// expected-note @+1 {{could not fold operation}}
#assert(evaluate(intExpr: .int(5)) == 5)
// expected-error @+2 {{#assert condition not constant}}
// expected-note @+1 {{could not fold operation}}
#assert(evaluate(intExpr: .add(.int(5), .int(6))) == 11)
// expected-error @+2 {{#assert condition not constant}}
// expected-note @+1 {{could not fold operation}}
#assert(evaluate(intExpr: .add(.multiply(.int(2), .int(2)), .int(3))) == 7)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the interpreter crash here, or just throws a diagnostics saying assert condition is not constant? If it is the latter case, can we add it here. It doesn't bring a lot of value to the test case, except for ensuring that the interpreter gives up gracefully (and possibly with a good diagnostics eventually).

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah, it makes a diagnostic. I have uncommented them.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcrasi Thanks for fixing all of this. LGTM

// Test address-only enums.
protocol IntContainerProtocol {
var value: Int { get }
}

struct IntContainer : IntContainerProtocol {
let value: Int
}

enum AddressOnlyEnum<T: IntContainerProtocol> {
case double(_ value: T)
case triple(_ value: T)
}

func evaluate<T>(addressOnlyEnum: AddressOnlyEnum<T>) -> Int {
switch addressOnlyEnum {
case .double(let value):
return 2 * value.value
case .triple(let value):
return 3 * value.value
}
}

#assert(evaluate(addressOnlyEnum: .double(IntContainer(value: 1))) == 2)
#assert(evaluate(addressOnlyEnum: .triple(IntContainer(value: 1))) == 3)