expression checking for @compilerEvaluable funcs #17256

marcrasi · 2018-06-15T21:15:23Z

It's mostly just a list of allowed expressions. DeclRefExpr has interesting logic.

jckarter · 2018-06-15T21:19:40Z

Would it be better to leave this analysis to the SIL pass that evaluates compiler-evaluable expressions? That way, as the constant evaluator improves, we don't need to update two places.

marcrasi · 2018-06-15T21:21:18Z

@swift-ci please test tensorflow

marcrasi · 2018-06-15T21:53:19Z

@jckarter The reason we're trying it this way is that there may be some types of code that we don't want to allow in general but that happen to generate evaluable SIL in special cases. Leaving the evaluation to the SIL pass would let these special cases through. This would make it hard for users to understand what is allowed. Even worse, changes in the interpreter might break these special cases.

Admittedly, I don't have a specific example of such a thing. I'll see if I can think of one.

Also, the SIL evaluation pass doesn't currently check code that it doesn't execute (e.g. @compilerEvaluable functions that are not called, and branches that are not taken). Though this can probably be changed.

lattner · 2018-06-16T21:41:22Z

@jckarter we're not super tied to this and such AST level analysis has limitations, but there is a nice advantage to it: we can diagnose obviously non-constexpr cases without having to evaluate a static assert that uses it. This seems like a pretty nice thing.

You're right that this requires that we keep it in sync with the SIL level logic.

lattner · 2018-06-16T21:44:22Z

lib/Sema/TypeCheckCompilerEvaluable.cpp

Err, this is very very stubbed out... if this is what you want for this patch, please make this the subject of your next patch :-)

lattner · 2018-06-16T21:46:00Z

lib/Sema/TypeCheckCompilerEvaluable.cpp

Please file a JIRA tracking this so we can discuss it when the implementation is further baked. I don't want these to fall off the radar. I think they should be handleable with the @compilerEvaluable(magicBehavior) feature that we'll need anyway.

done: https://bugs.swift.org/browse/SR-8035

jckarter · 2018-06-18T17:16:37Z

It seems to me that the model we'd ultimately want for this is that constant expressions are allowed based on the transitive compiler-evaluability of the operations they perform, not the syntactic forms the code happens to use at the AST level. Transparent inlining would be the primary mandatory pass I can think of that would lead to potentially confusing SIL-level behavior, but it seems to me that this should eventually supplant transparent. I can see the point that you can raise an error sooner for things that obviously aren't constexpr at the AST level, but that feels like a brittle thing to be checking for at the AST level as the model expands (especially the whitelisting of certain types and builtins that are allowed). In general I feel like for erroneous code it's better to spend more time providing more accurate diagnostics than to try to peephole diagnostics at multiple levels; in other places where we've attempted this, like casting diagnostics, we've had persistent problems keeping the behavior at different levels of the system in sync.

lattner · 2018-06-18T18:55:47Z

I can understand what you mean. The evaluator is really powerful in practice with the stdlib.

What is your opinion about using a new attribute (@compilerEvaluable or whatever) as a statement of intent above for public functions above and beyond @inlinable? Do you think it makes for an API author to make a declaration that "future versions of this API will not start doing non-constexpr things"? If that makes sense, would it makes sense for the compiler to help detect errors?

Can you think of another way to do this check?

mhong

Nice change! Got a few questions and minor suggestions.

mhong · 2018-06-16T01:01:03Z

test/Sema/compiler_evaluable.swift

mhong · 2018-06-18T16:57:41Z

lib/Sema/TypeCheckCompilerEvaluable.cpp

still keep this private? exposing it via the const getter getCompilerEvaluable() is a good API design.

I think it is still private?

Right. Looks good.

mhong · 2018-06-18T16:59:47Z

lib/Sema/TypeCheckCompilerEvaluable.cpp

when will this default case still be reachable (i thought ExprNodes.def would cover all cases)?

the #include "swift/AST/ExprNodes.def" above only defines cases for UNCHECKED_EXPR expressions

What do you mean? https://github.com/apple/swift/blob/tensorflow/include/swift/AST/ExprNodes.def contains more expr types than UNCHECKED_EXPR?

Yeah, ExprNodes.def contains a bunch of types of nodes. ExprNodes.def defines macros that expand them all to nothing. I define the UNCHECKED_EXPR macro so that it expands to a case, and then include ExprNodes.def. So only the UNCHECKED_EXPRs end up contributing cases to the switch.

Got it. Thanks for clarifying.

mhong · 2018-06-18T20:23:40Z

lib/Sema/TypeCheckCompilerEvaluable.cpp

dyn_cast_or_null instead of dyn_cast is used heer because the input can be NULL -- what would be an example?

when the parent is not an expression, it's null. for example, the top expression node for the condition of an if statement has the if statement as the parent.

mhong · 2018-06-18T20:24:39Z

lib/Sema/TypeCheckCompilerEvaluable.cpp

what does "compiler representable" mean -- is it defined/documented somewhere?

the proposal describes it in this section: https://gist.github.com/marcrasi/b0da27a45bb9925b3387b916e2797789#proposed-solution---intuitive-approach

I also plan to add more detailed comments describing "compiler representable" in a PR where I implement a real check for it.

Thanks. Why is float not compiler representable?

Float's not representable because we didn't propose to support it in the proposal. I think there are no fundamental difficulties with it -- we can just add the support whenever we want.

jckarter · 2018-06-18T23:05:48Z

@lattner

What is your opinion about using a new attribute (@compilerevaluable or whatever) as a statement of intent above for public functions above and beyond @inlinable? Do you think it makes for an API author to make a declaration that "future versions of this API will not start doing non-constexpr things"? If that makes sense, would it makes sense for the compiler to help detect errors?

That's the model that makes sense to me for public API. AIUI that's what you were proposing on swift-evolution, right?

lattner · 2018-06-19T01:18:17Z

@jckarter The proposal as written (still very much up for debate) requires this attribute on all functions. I think it makes sense to scope it back to just being required on public functions, but that would mean that it would still be on all the stdlib stuff of interest (replacing @inlinable).

That said, we still have the pertinent question: if we have this attribute on public APIs, does it make sense for the compiler to do static checking to ensure that the method isn't doing anything unsupported, even if there is no call to it? If we should do that checking, how should it be implemented? Does it have to be perfect, or does it just have to be useful?

jckarter · 2018-06-19T01:52:50Z

@lattner I agree that it makes sense to only require the attribute on public declarations. It should be possible to check and diagnose the compiler-evaluability of the implementation while compiling the defining module, though, so importing modules don't have to. I think it'd be easier and more accurate to do that check by looking over the SIL than looking over the AST.

mhong · 2018-06-19T17:53:22Z

lib/Sema/TypeCheckCompilerEvaluable.cpp

also def UNCHECKED_EXPR later?

Ah I see swift/AST/ExprNodes.def undefs it at the end. It feels a bit strange that the responsibility is not at the call-site, but if that's the convention, i'm fine with it.

marcrasi · 2018-06-19T17:57:53Z

@swift-ci please test tensorflow

marcrasi · 2018-06-19T17:58:07Z

@swift-ci please test tensorflow

marcrasi · 2018-06-21T23:33:58Z

@swift-ci please test tensorflow

marcrasi · 2018-06-21T23:34:23Z

@swift-ci please test tensorflow

It's mostly just a list of allowed expressions. DeclRefExpr has interesting logic.

marcrasi added the tensorflow This is for "tensorflow" branch PRs. label Jun 15, 2018

marcrasi requested a review from lattner June 15, 2018 21:15

lattner reviewed Jun 16, 2018

View reviewed changes

lattner approved these changes Jun 16, 2018

View reviewed changes

marcrasi force-pushed the tensorflow branch from 7bea477 to 13f448c Compare June 18, 2018 19:46

marcrasi force-pushed the implement-checks branch from f267858 to 42508a0 Compare June 18, 2018 20:02

mhong suggested changes Jun 18, 2018

View reviewed changes

mhong approved these changes Jun 19, 2018

View reviewed changes

mhong reviewed Jun 19, 2018

View reviewed changes

marcrasi force-pushed the implement-checks branch from 48e900f to 2c5eac2 Compare June 19, 2018 17:57

expression checking for @compilerEvaluable funcs

bfa5c83

marcrasi force-pushed the implement-checks branch from 2c5eac2 to bfa5c83 Compare June 21, 2018 23:33

marcrasi merged commit 576b082 into swiftlang:tensorflow Jun 22, 2018

marcrasi added a commit that referenced this pull request Jun 22, 2018

expression checking for @compilerEvaluable funcs (#17256)

31e656c

It's mostly just a list of allowed expressions. DeclRefExpr has interesting logic.

marcrasi added a commit that referenced this pull request Jun 28, 2018

expression checking for @compilerEvaluable funcs (#17256)

337050a

It's mostly just a list of allowed expressions. DeclRefExpr has interesting logic.

expression checking for @compilerEvaluable funcs #17256

expression checking for @compilerEvaluable funcs #17256

Uh oh!

Conversation

marcrasi commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcrasi commented Jun 15, 2018

Uh oh!

marcrasi commented Jun 15, 2018

Uh oh!

lattner commented Jun 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jun 18, 2018

Uh oh!

lattner commented Jun 18, 2018

Uh oh!

mhong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jun 18, 2018

Uh oh!

lattner commented Jun 19, 2018

Uh oh!

jckarter commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcrasi commented Jun 19, 2018

Uh oh!

marcrasi commented Jun 19, 2018

Uh oh!

marcrasi commented Jun 21, 2018

Uh oh!

marcrasi commented Jun 21, 2018

marcrasi commented Jun 15, 2018 •

edited

Loading

jckarter commented Jun 15, 2018 •

edited

Loading