Track dependencies of SIL instructions on opened archetypes which they use. #2928

swiftix · 2016-06-07T00:56:40Z

What's in this pull request?

Resolved bug number: (SR-)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

Till now there was no way in SIL to explicitly express a dependency of an instruction on any opened archetypes used by it. This was a cause of many errors and correctness issues. In many cases the code was moved around without taking into account these dependencies, which resulted in breaking the invariant that any uses of an opened archetype should be dominated by the definition of this archetype.

This patch does the following:

Map opened archetypes to the instructions defining them, i.e. to open_existential instructions.
Introduce a helper class SILOpenedArchetypesTracker for creating and maintaining such mappingss.
Each SIL instruction which uses an opened archetype as a type gets an additional "typedef" operand representing a dependency of the instruction on this archetype. These typedef operands are an in-memory representation. They are not serialized. Instead, they are re-constructed when reading binary or textual SIL files.
SILVerifier was extended to conduct more thorough checks related to the usage of opened archetypes.

swiftix · 2016-06-07T01:00:23Z

@swift-ci Please test

swiftix · 2016-06-07T01:10:04Z

@atrick @rudkx Do you mind reviewing the new version of the opened archetype tracking? This time it is a complete implementation. Each instruction using an opened archetype has a dedicated typedef operand to represent a dependency on the definition of this archetype. No global/function-wide maps are build during inlining or cloning (except for SILGen and SIL readers where new instructions are generated out a thin air). Does it look like what we envisioned?

The most interesting changes are:

SILInstruction.h: New APIs for working with typedef operands
SILBuilder.h: It registers newly defined opened archetypes when creating open_existential instructions. It properly sets the typedef operands for instructions using opened archetypes, in particular for apply instructions.
SILCloner.h: performs re-mapping of opened archetypes, handles witness_method instructions, etc.
SILOpenedArchetypesTracker.h and SILOpenedArchetypesTracker.cpp: provide a helper class and helper functions to work with opened archetypes, register mappings from opened archetypes to their definitions, etc.

swiftix · 2016-06-07T17:34:04Z

@jckarter Joe, you may want to have a look as well.

rjmccall · 2016-06-07T17:53:39Z

include/swift/SIL/SILBuilder.h

+
+  // Build a set of typedef operands for an apply instruction. This is done by
+  // scanning the list of substitutions and collecting all opened archetypes.
+  void createApplyTypeDefs(SILInstruction *I, ArrayRef<Substitution> Subs) {


Hmm. Unfortunately, I think there's too much potential confusion with C++ to use the term "typedef" for this.

@rjmccall Yeah, may be you are right about this. Any suggestions for a better name? ArchetypeDef? ArchDef? ExistentialDef? OpenExistentialDef?

These are opened archetype operands that refer to opened existential values. We're not even creating special "def" values for archetypes yet, so let's not call them "defs". OpenedArchetype is fine.

atrick · 2016-06-08T23:59:15Z

I think most of the issues that I saw are going to be fixed after handling @rjmccall's suggestions. Let me summarize:

SILInstruction should only expose the fully initialized getOpenedArchetypeOperands() list. The underlying buffer is an internal memory management detail, and putting it in the public interface is confusing.
Use TailAllocatedOperandList for the opcodes that may need extra OpenedArchetypeOperands.
Using temporary Undef operands is too clever. It forces anyone who looks at SIL instruction creation logic to understand how the cloner works.
Make the abstraction used at the level of SIL builder and SIL instruction as simple as possible. The builder can contain a basic abstraction of a map from Archetype to OpenExistential. The only guarantee is that the map covers the archetypes that will be referenced by any instructions the builder is asked to create and are not trivially derived from the normal operand list. This is not too far from what you have, but I was confused about what was supposed to be in the tracker at any given point. I think we mainly need to comment that the map will track opened archetypes that were created directly by this builder, but may be seeded with entries provided by the client, for example in order to clone into a new context.
Have the cloner build (or accumulate) the Archetype->OpenExistential
map from the OpenedArchtypeOperands list in the old instructions
before passing it to the SIL builder. In this case, opened
archetypes are coming from two places. Unify the set, at least
logically, before passing it to the SIL builder.

swiftix · 2016-06-13T23:01:47Z

@atrick @rjmccall I re-factored the code to incorporate all of the feedback provide by your reviews. Please have a look and let me know if I missed anything.

swiftix · 2016-06-13T23:03:30Z

@swift-ci Please test

swiftix · 2016-06-13T23:22:59Z

Main changes in this version are:

SILGen and SIL deserializers use the SILOpenedArchetypesTracker, but constructors and create methods of SILInstruction objects use the SILOpenedArchetypesState, which is essentially a read-only API for looking up opened archetypes, which may come either from an SILOpenedArchetypesTracker or from the instruction being currently cloned.
SILBuilder does not try to construct any opened archetypes sets for instructions. Instead, it delegates this job to SILInstruction constructors and create methods, as @rjmccall suggested.
I got rid of all those clever tricks involving SILUndef.
Each instruction that may have opened archetypes operands, can have any number of those. This is achieved by tail-allocating those opened archetype operands.
No changes to the list of opened archetypes of a SILInstruction are possible after the instruction is constructed.

atrick · 2016-06-14T03:43:16Z

include/swift/SIL/SILCloner.h

+  auto newLookupType = getOpASTType(lookupType);
+  SILValue OpenedExistential;
+  if (Inst->hasOperand())
+    OpenedExistential = getOpValue(Inst->getOperand());


Could you be more explicit when getting the opened archetype operand by using getNumOpenedArchetypesOperands(), getOpenedArchetypesOperands?

It looks like this assumes that if the old instruction has an opened archetype operand, that the new one will too. I'm not sure that's right. If so, then we should have some assert.

Actually, with all the introduced machinery in place, we can now handle the WitnessMethodInst as any regular instruction that may have opened archetype operands. There is no need for any special handling.

atrick · 2016-06-14T06:35:58Z

Thanks Roman. I think this design makes sense, at least for now... I know it was very tricky to implement. I like that the set of operations that may need opened archetype operands are explicit, even thought it's awkward to always pass in their opened archetypes. It is ugly, but I think it would be a mistake to hide the ugliness with magic inside SILIntruction creation.

I like that the SILInstruction actually manages the operands and allocates memory up front, and that the builder tracks state across the creation of multiple instructions.

swiftix · 2016-06-14T07:21:43Z

@atrick Andy, thanks a lot for your review! It was very helpful!

I think I addressed (almost) all of your comments. I pushed my changes as a series of commits on top of the one that you just reviewed to make it easier for you to see what was changed. Most of these changes can be classified as renaming or dead code removal.

Once there are no further required changes, I'll squash the commits before merging.

swiftix · 2016-06-15T16:07:02Z

@swift-ci Please test

swiftix · 2016-06-21T21:02:54Z

I squashed all commits into one before merging.

swiftix · 2016-06-21T21:03:24Z

@swift-ci Please test

rjmccall · 2016-06-22T05:21:07Z

include/swift/SIL/SILInstruction.h

+  MutableArrayRef<Operand> getOpenedArchetypeOperands() {
+    if (!hasOpenedArchetypeOperands())
+      return {};
+    return { &getAllOperands()[0] + 1, getNumOpenedArchetypeOperands() };


This entire method is just:
return getAllOperands().slice(1);

rjmccall · 2016-06-22T05:42:23Z

Style comments aside, LGTM.

swiftix · 2016-06-22T18:57:24Z

I addressed all of @rjmccall comments and squashed the commits into one.

swiftix · 2016-06-22T18:57:38Z

@swift-ci Please test

…y use Till now there was no way in SIL to explicitly express a dependency of an instruction on any opened archetypes used by it. This was a cause of many errors and correctness issues. In many cases the code was moved around without taking into account these dependencies, which resulted in breaking the invariant that any uses of an opened archetype should be dominated by the definition of this archetype. This patch does the following: - Map opened archetypes to the instructions defining them, i.e. to open_existential instructions. - Introduce a helper class SILOpenedArchetypesTracker for creating and maintaining such mappings. - Introduce a helper class SILOpenedArchetypesState for providing a read-only API for looking up available opened archetypes. - Each SIL instruction which uses an opened archetype as a type gets an additional opened archetype operand representing a dependency of the instruction on this archetype. These opened archetypes operands are an in-memory representation. They are not serialized. Instead, they are re-constructed when reading binary or textual SIL files. - SILVerifier was extended to conduct more thorough checks related to the usage of opened archetypes.

swiftix · 2016-06-22T21:32:30Z

The last forced push just updated the time stamp of the commit which had the 26th of May as its date ;-)

swiftix · 2016-06-22T21:34:14Z

@atrick @rjmccall @jckarter Thanks a lot for reviewing this PR! I've updated the PR according to all of your comments. Merging now.

rjmccall reviewed Jun 7, 2016
View reviewed changes

swiftix force-pushed the wip-opened-archetypes-v3 branch from 1dc123e to d7ec9da Compare June 13, 2016 22:58

swiftix force-pushed the wip-opened-archetypes-v3 branch from d7ec9da to edc71bd Compare June 13, 2016 23:40

atrick reviewed Jun 14, 2016
View reviewed changes

swiftix force-pushed the wip-opened-archetypes-v3 branch from da31d65 to b803b7f Compare June 21, 2016 21:02

swiftix changed the title ~~[don't merge] Track dependencies of SIL instructions on opened archetypes which they use.~~ Track dependencies of SIL instructions on opened archetypes which they use. Jun 21, 2016

rjmccall reviewed Jun 22, 2016
View reviewed changes

swiftix force-pushed the wip-opened-archetypes-v3 branch from 5b7b6a6 to 0a3882b Compare June 22, 2016 18:56

swiftix force-pushed the wip-opened-archetypes-v3 branch from 0a3882b to 8ef8bb4 Compare June 22, 2016 21:31

swiftix merged commit d10aa3f into swiftlang:master Jun 22, 2016

Track dependencies of SIL instructions on opened archetypes which they use. #2928

Track dependencies of SIL instructions on opened archetypes which they use. #2928

Uh oh!

Conversation

swiftix commented Jun 7, 2016

What's in this pull request?

Resolved bug number: (SR-)

Uh oh!

swiftix commented Jun 7, 2016

Uh oh!

swiftix commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swiftix commented Jun 7, 2016

Uh oh!

rjmccall Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

swiftix Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

atrick Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

atrick commented Jun 8, 2016

Uh oh!

swiftix commented Jun 13, 2016

Uh oh!

swiftix commented Jun 13, 2016

Uh oh!

swiftix commented Jun 13, 2016

Uh oh!

atrick Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

swiftix Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

atrick commented Jun 14, 2016

Uh oh!

swiftix commented Jun 14, 2016

Uh oh!

swiftix commented Jun 15, 2016

Uh oh!

swiftix commented Jun 21, 2016

Uh oh!

swiftix commented Jun 21, 2016

Uh oh!

rjmccall Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

rjmccall commented Jun 22, 2016

Uh oh!

swiftix commented Jun 22, 2016

Uh oh!

swiftix commented Jun 22, 2016

Uh oh!

swiftix commented Jun 22, 2016

Uh oh!

swiftix commented Jun 22, 2016

Uh oh!

Uh oh!

swiftix commented Jun 7, 2016 •

edited

Loading