Skip to content
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

[SIL]Record conformances when parsing init_existential_metatype #786

Merged
merged 1 commit into from Jan 8, 2016
Merged

[SIL]Record conformances when parsing init_existential_metatype #786

merged 1 commit into from Jan 8, 2016

Conversation

ezephir
Copy link

@ezephir ezephir commented Dec 26, 2015

This partially resolves bug SR-247.

@ezephir
Copy link
Author

ezephir commented Dec 26, 2015

Questions for the review:

  1. How should I approach testing this?
    • Textual SIL doesn't directly report conformances.
    • I might be able to verify this by looking for witness_tables, but as best as I can tell, they're only emitted when dealing with nominative types. I'm also concerned that the resulting tests would be fragile and depend on the behavior of SILOpt. (e.g. in the event that I was to provide incomplete witness_table information in order to see if SILOpt notices the conformances in the instruction and fills it in automatically).
  2. On a related note, the verifier doesn't appear to check that the operand actually conforms to the protocol the existential type is made from. Am I correct in treating this as an entirely separate issue from this bug?
  3. Over at SILParser, inbetween line 3319 and 3328, we have this comment:

// FIXME: Conformances in InitExistentialRefInst is currently not included
// in SIL.rst.

However, it does seem to be discussed. I'm looking at the sections at line 3257 and line 3387 of SIL.rst. Is this comment out-of-date or am I misunderstanding what is missing?

@jckarter
Copy link
Member

For (1), an IRGen test that checks whether the initialized existential metatype is formed with the correct witness table for the conformance should work.

@jckarter
Copy link
Member

Looks like you're right about (2) and (3). The verifier can be improved separately, and the TODO looks out of date.

@ezephir
Copy link
Author

ezephir commented Dec 26, 2015

Awesome. Thank you!

I've filed SR 390 about the verifier.

@ezephir
Copy link
Author

ezephir commented Dec 29, 2015

Okay. Changes since last time:

  • Fix to better determine protocol conformance. My previous submission was entirely incorrect. The new code is lifted from CSApply::coerceExistential() lines 3907-3915.
  • I've converted the existential metatypes case to SIL. This seems to be the last blocker.
  • Everything has been rebased against master.

New questions for this review round:

  1. What is the proper nomenclature for a type that's not a metatype? I want to say an instance type (based on the MetatypeType API), but that doesn't seem right.
  2. Canonical or Raw SIL for this test case? I chose canonical to go with the majority.
  3. Mangled or friendly function labels? Both seem to appear in IRGen test cases.
  4. How do I determine the proper RUN invocation for this test. I've tried using what looks like the standard line // RUN: %target-swift-frontend -emit-ir %s | FileCheck %s but without -primary-file, not all of the functions are emitted.
  5. Where would you recommend starting to dig to understand the intention of the -primary-file flag? I'm quite confused. I seem to need it to get -emit-ir to work, but if I combine it with -emit-silgen on the original Swift test-case, the output omits class and protocol definitions.

Thanks again for all of the help!
(edited to fix formatting)

@jckarter jckarter self-assigned this Jan 6, 2016
@jckarter
Copy link
Member

jckarter commented Jan 6, 2016

  1. 'Instance type' is used to refer to the instance type relative to a metatype (the T in T.Type). "Non-metatype type" works as a general term.
  2. For IRGen tests, you want canonical SIL. Otherwise, the diagnostic passes will run before IRGen, which interferes with the purity of the test.
  3. I prefer friendly function names in IRGen tests to the maximum degree possible. There are some unfortunate places in IRGen where we still hardcode manglings, particularly around ObjC entry points.

For 4 and 5, I defer to @jrose-apple. I'm not sure why -primary-file is significant here.

@jrose-apple
Copy link
Contributor

-primary-file is how the frontend distinguishes "whole-module optimization" compilation from normal "multi-file" compilation.

# Compiles a.swift, b.swift, and c.swift together
swiftc a.swift b.swift c.swift

# Just compiles b.swift, though it can use declarations from a.swift and c.swift
swiftc a.swift -primary-file b.swift c.swift

This changes what declarations are considered dead code: if you're compiling everything together, anything that's unused and not public is dead and eliminated before we get to IRGen. You can either mark more things public, or not worry about the extra class and protocol definitions.

@ezephir
Copy link
Author

ezephir commented Jan 7, 2016

Thank you @jckarter and @jrose-apple for the explanation!

I've updated this PR to:

  • Use human friendly names
  • Use use appropriate access control
  • Trim excess comments and the unused function signatures
  • Rebase against master and adopt some recent SIL changes

The new changes pass the validation suite cleanly on OS X 10.10 and are ready for inclusion or further feedback.

Convert a test in order to test the SIL parser and avoid the front end. The
updated test verifies that conformances for the init_existential_metatype
instruction are calculated correctly. The matching functionality was added in
commit 2df6880.
@ezephir
Copy link
Author

ezephir commented Jan 8, 2016

Commit 2df6880 landed code changes equivalent to those in this PR. The newly converted test still adds value though, because it directly exercises the newly added code path and it avoids the frontend.

I've reverted the code change, rebased against master, and changed the commit message to match. This updated PR passes cleanly on OS X 10.10 against the new code.

rjmccall added a commit that referenced this pull request Jan 8, 2016
[SIL]Record conformances when parsing init_existential_metatype
@rjmccall rjmccall merged commit 432018b into apple:master Jan 8, 2016
@rjmccall
Copy link
Member

rjmccall commented Jan 8, 2016

Thanks, looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants