const evaluator: enums #21869
const evaluator: enums #21869
Conversation
| #assert(weighPet(pet: .cat(2)) == 2) | ||
| // expected-error @+1 {{assertion failed}} | ||
| #assert(weighPet(pet: .cat(2)) == 3) | ||
| #assert(weighPet(pet: .dog(9, 10)) == 19) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ASTContext &astContext) { | ||
| assert(decl && payload.isConstant()); | ||
| auto rawMem = astContext.Allocate(sizeof(EnumWithPayloadSymbolicValue), | ||
| alignof(EnumWithPayloadSymbolicValue)); |
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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. :-)
|
@marcrasi I have just added a comment on adding a test case. Otherwise, it LGTM. Thanks a lot for doing this. |
| case add(_ lhs: IntExpr, _ rhs: IntExpr) | ||
| case multiply(_ lhs: IntExpr, _ rhs: IntExpr) | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks like there are some extra spaces here, and also in the code below
There was a problem hiding this comment.
Oh sorry, the red bars were underscores. It looked like GitHub was flagging was extra spaces (diffs)
| // #assert(evaluate(intExpr: .int(5)) == 5) | ||
| // #assert(evaluate(intExpr: .add(.int(5), .int(6))) == 11) | ||
| // #assert(evaluate(intExpr: .add(.multiply(.int(2), .int(2)), .int(3))) == 7) | ||
|
|
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Oh yeah, it makes a diagnostic. I have uncommented them.
There was a problem hiding this comment.
@marcrasi Thanks for fixing all of this. LGTM
marcrasi
force-pushed
the
marcrasi-const-evaluator-enums
branch
from
cdd9b77 to
fb45802
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please test and merge |
1 similar comment
|
@swift-ci Please test and merge |
Implements enums in the const evaluator.