constant interpreter: support evaluating generic functions #19662

marcrasi · 2018-10-02T04:40:47Z

This adds support for evaluating generic functions to the constant interpreter.

This makes SILModule::getSILLoader() and getWitnessMethodSubstitutions public so that the interpreter can access them. Maybe we should do something more proper before merging this. Suggestions about what exactly to do would be very helpful!

This is a part of the #19579 mega-patch. See there if you're interested in what comes next.

slavapestov · 2018-10-02T20:09:29Z

lib/SILOptimizer/Utils/Devirtualize.cpp

@@ -874,7 +874,9 @@ getWitnessMethodSubstitutions(
      witnessThunkSig);
 }

-static SubstitutionMap
+// This function is made public so it can be used by the constexpr propagation


Instead, why not make a public function which devirtualizes an apply of a witness_method, and then the constant evaluator only has to worry about calls of static function references?

Mandatory inlining has something similar as well, so at this point it looks like this is adding a fourth copy of a very similar piece of code.

I set out to do this and discovered an obstacle: I can't actually mutate the SIL to have static function references (like the other APIs in Devirtualize.h do) because the function containing the apply can still be totally generic. The interpreter knows all the concrete types because it arrived at the apply by following a specific path.

I could create a Devirtualize.h public function that takes an ApplyInst and the current substitutions and returns a devirtualized ApplyInst without replacing the existing one. That might be confusingly different from the other Devirtualize.h APIs, and it also wastefully allocates an ApplyInst that is used for a very temporary purpose.

What's your suggestion?

@ravikandhadai @marcrasi I think refactoring the devirtualizer to return a SILFunction * together with a SubstitutionMap might be the way to go. You're right that allocating a new ApplyInst might be wasteful if you're not actually mutating the SIL.

@slavapestov I would like clarify what's happening here a bit more clearly to bring out challenges in sharing any more logic with the devirtualizer. (@marcrasi correct me if I am wrong anywhere.)

Firstly, the evaluator already has aSILFunction at this point. It knows the target. But, it only needs a substitution map. There is an important reason why the evaluator cannot just pass an Apply Instruction to the devirtualization methods and expect it to find a SILFunction and Substitution map. As opposed to a typical devirtualization, the "protocol conformance" that can be obtained from the apply instruction may not be concrete. That is, the devirtualizer cannot devirtualize it at all, i.e, neither compute the target SILFunction nor the substitution map, because the apply instruction may only have an abstract conformance. The evaluator alone can construct the concrete protocol conformance, because it was interpreting the program interprocedurally and figured out the concrete types of the generics/existentials. So it actually constructs a "concrete conformance" and calls getWitnessMethodSubstitutions passing in the concrete conformance it constructed. (The evaluator is tracking a substitution map as a part of the interpreter state through procedures to be able to construct this concrete conformance.)

So, if we factor out any code at all from the devirtualizer, the factored code must accept a protocol conformance as a parameter (because only the evaluator knows how to construct it and it cannot be extracted from the ApplyInst).
The interface of the factored code will look much like the interface of devirtualizeWitnessMethod and would perform the first two lines of it, which essentially is a call to getWitnessMethodSubstitutions.
We cannot share anything with tryDevirtualizeWitnessMethod as the construction of SILFunction and ProtocolConformanceRef are very different between devirtualization and the evaluator.

It seems the code is not duplicating any functionality here. It is constructing a concrete conformance from its internal state in lines 601 to 617 (but this code should be commented well and can be compressed a bit), which cannot go anywhere else, and reuses a devirtualizer helper function that constructs a substitution map, given a concrete protocol conformance and SILFunction etc.

Let me know what you think.

slavapestov · 2018-10-02T20:09:54Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+    auto conf = confResult.getValue();
+    auto &module = wmi->getModule();
+
+    // Look up the conformance's withness table and the member out of it.


Typo: "witness"

marcrasi · 2018-10-08T18:40:57Z

@slavapestov thanks for the suggestion! I will try it out in the tensorflow branch, and then propagate it to this PR. Hopefully in a few days.

Also fyi, I made a nontrivial change to getSingleWriterAddressValue in the tensorflow branch in #19705. I will propagate that change into this PR at the same time that I propagate @slavapestov's suggestion.

ravikandhadai · 2018-10-08T20:26:35Z

I made a nontrivial change to getSingleWriterAddressValue in the tensorflow branch in #19705. I will propagate that change into this PR

@marcrasi Sounds great. Thanks.

slavapestov · 2018-10-16T08:01:11Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+    } else {
+      auto requirementSig = AI.getOrigCalleeType()->getGenericSignature();
+      callSubMap =
+          SubstitutionMap::get(requirementSig, apply->getSubstitutionMap());


Just using apply->getSubstitutionMap() should be sufficient -- you don't have to build a new one since it should have the same generic signature.

@ravikandhadai This still needs to be addressed

@slavapestov Thanks for pointing this out. This copy can be removed.

slavapestov · 2018-10-16T08:03:32Z

lib/SILOptimizer/Utils/ConstExpr.cpp

@@ -134,10 +132,42 @@ Type ConstExprFunctionState::simplifyType(Type ty) {
  return substitutionMap.empty() ? ty : ty.subst(substitutionMap);
 }

+// TODO: refactor this out somewhere sharable between autodiff and this code.
+static void lookupOrLinkWitnessTable(ProtocolConformanceRef confRef,


I guess I don't really understand why this PR is adding so much new code (lookupOrLinkWitnessTable() and getWitnessMethodSubstitutions()) when very similar logic already exists in the devirtualizer.

You bring up a very good point that while the devirtualizer rewrites instructions in place, the evaluator looks at but does not mutate the AST. However it should be possible to come up with some common pieces that can be reused -- for example, a common utility returning the SILFunction and SubstitutionMap for a devirtualized call. The optimizer can build an ApplyInst from that, and the evaluator can proceed to evaluate the body of the SILFunction.

I'm generally pessimistic about PRs that add TODO/FIXME comments because in practice we rarely see them get addressed, as everyone is keen to move on to the next feature...

I agree that I should deal with cleanup TODOs before merging this PR. They're in the PR now to help me collect suggestions about how to do the cleanups -- I don't intend to try to get this PR landed before doing something about them.

I will look through the devirtualizer code and see what common pieces I can come up with, and I'll get back to you if I run into any concrete obstacles or things that I don't understand.

Sounds good!

I have some suggestions for addressing @slavapestov 's concerns. It seems to me that lookupOrLinkWitnessTable() functionality is partly available in the SILLinkerVisitor::visitProtocolConformance. Apparently, this is the only other place where we create a witness table declaration for a protocol conformance in case we do not find a witness table for the conformance. (Even though somewhat similar logic exist in other places, it seems to me that there is an assumption that the witness table declaration must exist for a protocol in other places.) But, much of what visitProtocolConformance does, such as deserializing any witness methods and visiting related protocol conformances, are not needed for the evaluator. (Deserialization of the witness method would happen later during evaluation, only if the function is actually invoked during evaluation.) Therefore, it seems to me that we could factor out this common functionality, shown below, from visitProtocolConformance.

SILWitnessTable *lookupOrCreateWitnessTable(ProtocolConformance *C, bool mustDeserialize, SILModule &Mod) { SILWitnessTable *WT = Mod.lookUpWitnessTable(C, true); // If we don't find any witness table for the conformance, // try to create a witness table declaration and try again. if (!WT) { Mod.createWitnessTableDeclaration(C, getLinkageForProtocolConformance( C->getRootNormalConformance(), NotForDefinition)); // Adding the declaration may allow us to now deserialize the body. // Force the body if we must deserialize this witness table. if (mustDeserialize) { WT = Mod.lookUpWitnessTable(C, true); assert(WT && WT->isDefinition() && "unable to deserialize witness table when we must?!"); } else { return WT; } } return WT; }

I am not sure where this function should go (Perhaps in the SILLinker.cpp itself or SILModule.cpp?). If we have this factored out, both the evaluator and SILLinkerVisitor::visitProtocolConformance can use it.

Do you really want to create the witness table if it cannot be deserialized? You won't be able to devirtualize the method if the witness table is empty.

Check out #21476 -- I updated lookUpWitnessTable() to create the declaration if one doesn't exist, so you can simplify lookupOrLinkWitnessTable() or perhaps remove it entirely.

Thanks a lot @slavapestov. This is awesome.

ravikandhadai · 2018-12-19T01:23:27Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+  for (auto &entry : newTable->getEntries())
+    if (entry.getKind() == SILWitnessTable::WitnessKind::Method)
+      entry.getMethodWitness().Witness->setLinkage(linkage);
+}


@marcrasi I have a quick question. I am wondering why lines 153 to 156 that updates the linkage of the witness methods necessary. The comment seems to imply that the linkage is updates as they have a default shared linkage. Just wondering why that could be a problem? Could we make a test case that can produce an error without this?

Yeah, you really don't want to be doing this -- there must be another issue elsewhere that needs to be fixed.

slavapestov · 2018-12-20T19:01:54Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+      // current type space.
+      auto conformance = calleeFnType->getWitnessMethodConformance();
+      auto selfTy = conformance.getRequirement()->getSelfInterfaceType();
+      auto conf = substitutionMap.lookupConformance(


I think you want AI->getSubstitutionMap().lookupConformance()

@slavapestov Thanks for pointing this out. You are right that this code is quite broken. @marcrasi I could make a test case (shown below) that crashes it.
I think we should use:
apply->getSubstitutionMap().subst(substitutionMap).lookupConformance(...)
This is because apply->getSubstitutionMap() would create a map in which Self type of the protocol will be bound to the receiver type of the call site. And, composing it with substitutionMap will map it to the actual concrete receiver type tracked by the interpreter. Therefore, we would get the right concrete conformance. This seems to fix it.

Just for the reference, the test case that bring out this problem is shown below:

protocol Proto { func amethod<U>(_ u: U) -> Int } struct S : Proto { func amethod<U>(_ u: U) -> Int { return 0 } } func callMethod<U, T: Proto>(_ a: T, _ u: U) -> Int { return a.amethod(u) } func testProtocolMethod() { let s = S() #assert(callMethod(s, 10) == 0) }

slavapestov · 2018-12-20T19:04:44Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+    // into the caller's world.
+    SubstitutionMap callSubMap;
+    if (calleeFnType->getRepresentation() ==
+        SILFunctionType::Representation::WitnessMethod) {


If you have a class method, you also need to do some remapping for vtable substitutions. Eg if I have class G<T, U> : C<(T, U), Int>, and I'm calling a method on an instance of G<String, Float>, but its actually implemented on C, I have to remap the substitution map to C<(String, Float), Int>.

Should we assert here that the function is not a class method. Is there a way to do that?

Looks like this may work:
assert(!calleeFnType->getSelfInstanceType()->getClassOrBoundGenericClass(), "class methods are not supported")

Perhaps this is better:
assert(!calleeFnType->hasSelfParam() || !calleeFnType->getSelfInstanceType()->getClassOrBoundGenericClass())

ravikandhadai · 2018-12-22T01:19:33Z

@marcrasi I have pushed one commit, so that it would easier to merge this PR once you can take look at this. The new commit updated to work with @slavapestov's changes, fixes some bugs, adds more test cases, and improves the comments a bit.

ravikandhadai · 2018-12-22T01:26:52Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+      // initializer, so we skip it (`getSingleWriterAddressValue(teai)` below
+      // will return non-const on it). %183 is a good initializer and can be
+      // const-evaluated (by const-evaluating %114).
+      auto *use = teai->getSingleUse();


@marcrasi should copy_addr %114 to [initialization] %183 : $*Int32 in the comments go after %183 = tuple_element_addr %179 : $*(Int32, Int32, Int32, Int32), 3 ?

ravikandhadai · 2018-12-22T01:34:04Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+      auto tupleEltAddrValue = getSingleWriterAddressValue(teai);
+      if (!tupleEltAddrValue.isConstant())
+        continue;
+      // If the tuple elt is indeed a const, we write it into (the appropriate


@marcrasi I have a suggestion here. It seems !tupleEltAddrValue.isConstant() is used to imply that tupleEltAddrValue has no writers at all, and is only read. (Like the %191 example presented above.) But, we are sort of overloading the meaning of !isConstnat() here to mean "no writers found". I wonder whether this return value could be make explicit by returning e.g. a boolean flag from the function getSingleWriterAddressValue() to indicate that no writer was found. This code can them check for this flag.

I was able to write a SIL test that exposes a bug that this causes! Will change it and add the test.

ravikandhadai · 2018-12-22T01:37:44Z

@marcrasi I have left a couple of minor comments I noticed while making the changes. Let me know if they make sense. Except that, this PR looks good to me.

marcrasi · 2019-01-02T23:22:09Z

Sorry for the delay! I'm starting to look now.

ravikandhadai · 2019-01-02T23:30:35Z

Cool. Thanks for working on this. Feel free to ping me if I could clarify any comments here or help with something.

marcrasi · 2019-01-04T01:16:50Z

Thanks for all the help with this one, @slavapestov and @ravikandhadai :)

It looks good to me now. I made a few changes:

asserted that the method is not a class method
added SIL tests exercising all the getSingleWriterAddressValue behavior
made the getSingleWriterAddressValue change suggested by @ravikandhadai, which fixed a bug. (and I added a test exposing this bug)

ravikandhadai · 2019-01-04T01:27:33Z

That's wonderful!

ravikandhadai · 2019-01-04T01:27:50Z

@swift-ci Please smoke test

ravikandhadai · 2019-01-04T01:32:17Z

@swift-ci Please smoke test

ravikandhadai · 2019-01-04T02:04:56Z

@marcrasi Thanks for all your help and effort. I think, once this gets merged, what remains is Strings, Arrays, Enums and Floats. Would it be possible for you to create PRs for these? I foresee that String and Arrays might involve more reviews than the other two, and they are introducing support for the well-known functions. Perhaps it is better to start that off, whenever you can.

marcrasi · 2019-01-04T02:08:31Z

Yes, that's what remains. I will try to start PRs for some of them right now.

marcrasi · 2019-01-04T03:47:10Z

I just updated this PR with the changes from #21628.

marcrasi · 2019-01-04T04:10:25Z

@swift-ci Please smoke test

marcrasi · 2019-01-04T04:10:36Z

@swift-ci Please smoke test

devincoughlin

LGTM. Thank you for adding more tests and documentation!

ravikandhadai · 2019-01-04T22:46:57Z

@swift-ci Please test and merge

ravikandhadai · 2019-01-04T22:49:56Z

@swift-ci Please test and merge

marcrasi requested review from devincoughlin and ravikandhadai October 2, 2018 04:40

marcrasi mentioned this pull request Oct 2, 2018

Compile-time interpretation #19579

Closed

slavapestov reviewed Oct 2, 2018

View reviewed changes

slavapestov reviewed Oct 16, 2018

View reviewed changes

marcrasi mentioned this pull request Nov 1, 2018

Add TensorGroup requirement to Dataset API #20197

Merged

marcrasi force-pushed the marcrasi-const-evaluator-part-2 branch from 3328daf to 30bda99 Compare November 30, 2018 21:45

marcrasi force-pushed the marcrasi-const-evaluator-part-2 branch 2 times, most recently from c721146 to 2b756b8 Compare December 12, 2018 04:44

marcrasi mentioned this pull request Dec 12, 2018

constant interpreter: addresses and memory #19657

Merged

marcrasi force-pushed the marcrasi-const-evaluator-part-2 branch from 2b756b8 to 8dcf89c Compare December 15, 2018 04:21

ravikandhadai reviewed Dec 19, 2018

View reviewed changes

slavapestov reviewed Dec 20, 2018

View reviewed changes

ravikandhadai force-pushed the marcrasi-const-evaluator-part-3 branch 2 times, most recently from a11b51f to db7481a Compare December 22, 2018 01:12

ravikandhadai reviewed Dec 22, 2018

View reviewed changes

marcrasi changed the base branch from marcrasi-const-evaluator-part-2 to master January 2, 2019 23:17

marcrasi force-pushed the marcrasi-const-evaluator-part-3 branch 3 times, most recently from 17f6831 to 01cac51 Compare January 4, 2019 01:14

const interpreter generics

867d96d

marcrasi force-pushed the marcrasi-const-evaluator-part-3 branch from 01cac51 to 867d96d Compare January 4, 2019 03:46

devincoughlin approved these changes Jan 4, 2019

View reviewed changes

swift-ci merged commit f98d7bd into master Jan 5, 2019

compnerd deleted the marcrasi-const-evaluator-part-3 branch May 31, 2025 20:32

constant interpreter: support evaluating generic functions #19662

constant interpreter: support evaluating generic functions #19662

Uh oh!

Conversation

marcrasi commented Oct 2, 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!

Choose a reason for hiding this comment

Uh oh!

ravikandhadai Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcrasi commented Oct 8, 2018

Uh oh!

ravikandhadai commented Oct 8, 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!

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!

ravikandhadai Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ravikandhadai commented Dec 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikandhadai Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikandhadai commented Dec 22, 2018

Uh oh!

marcrasi commented Jan 2, 2019

Uh oh!

ravikandhadai Dec 19, 2018 •

edited

Loading

ravikandhadai Dec 21, 2018 •

edited

Loading

ravikandhadai Dec 22, 2018 •

edited

Loading