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

SemIR fidelity when representing rewrite semantics #3833

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

chandlerc
Copy link
Contributor

@chandlerc chandlerc commented Mar 29, 2024

The toolchain's Semantic IR should start off modeling the full,
complex, and rich library-based and generic extension point semantics of Carbon
without eliding any layers or rewrites for compile time efficiency. We shouldn't
front-load elision or optimization when implementing the designs.

Once we have a full-fidelity implementation, we should work to build an
efficient elision, short-circuit, or common case simplification into the design
itself sufficient to make the SemIR model efficient. Only if we cannot find a
reasonable approach for that should we diverge the SemIR model to optimize its
efficiency, and we should preserve full fidelity in an optional mode.

@chandlerc chandlerc added proposal A proposal proposal draft Proposal in draft, not ready for review labels Mar 29, 2024
@chandlerc chandlerc force-pushed the semir-rewrite-fidelity branch 2 times, most recently from 94a605c to 96f3fcb Compare March 29, 2024 07:27
@chandlerc chandlerc marked this pull request as ready for review March 29, 2024 07:28
@github-actions github-actions bot added proposal rfc Proposal with request-for-comment sent out and removed proposal draft Proposal in draft, not ready for review labels Mar 29, 2024
proposals/p3833.md Outdated Show resolved Hide resolved
operators? This proposal suggests that _initially_, the implementation should
aim to fully model the rewrite-based dispatch through interfaces in the prelude.
That is, each use af `x + y` should turn into roughly equivalent SemIR as would
be used to model the rewritten semantics of `x.(Core.AddWith(typeof(y)).Op)(y)`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zygoloid Is this syntax allowed?

Copy link
Contributor

@zygoloid zygoloid Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roughly, yes, I think so. I see a couple of issues here:

  • typeof doesn't exist (yet).
  • I would not expect us to need Core to be in scope -- I think we mean "the AddWith that lives in the prelude" rather than "the result of name lookup into the name Core" here. In Arithmetic expressions #1083, the rule was (supposed to be) that we look for an implementation of the interface from the standard library, not that we do a name lookup to find the name Core and look for these various names within whatever we find.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the syntax is now clarified as imagined syntax and not doing weird things with name lookup.

That is, each use af `x + y` should turn into roughly equivalent SemIR as would
be used to model the rewritten semantics of `x.(Core.AddWith(typeof(y)).Op)(y)`.
This in turn would dispatch to an exact-type implementation which would provide
any implicit conversions, and so-on.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the alternative(s) that is/are being decided against in terms of this example as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could point out that this happens even when adding together (say) i32s.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense, I think done.

testing/file_test/autoupdate.cpp Outdated Show resolved Hide resolved
proposals/p3833.md Show resolved Hide resolved
@chandlerc
Copy link
Contributor Author

(the brief diff issues should be fixed now, sorry for that)

operators? This proposal suggests that _initially_, the implementation should
aim to fully model the rewrite-based dispatch through interfaces in the prelude.
That is, each use af `x + y` should turn into roughly equivalent SemIR as would
be used to model the rewritten semantics of `x.(Core.AddWith(typeof(y)).Op)(y)`.
Copy link
Contributor

@zygoloid zygoloid Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roughly, yes, I think so. I see a couple of issues here:

  • typeof doesn't exist (yet).
  • I would not expect us to need Core to be in scope -- I think we mean "the AddWith that lives in the prelude" rather than "the result of name lookup into the name Core" here. In Arithmetic expressions #1083, the rule was (supposed to be) that we look for an implementation of the interface from the standard library, not that we do a name lookup to find the name Core and look for these various names within whatever we find.

That is, each use af `x + y` should turn into roughly equivalent SemIR as would
be used to model the rewritten semantics of `x.(Core.AddWith(typeof(y)).Op)(y)`.
This in turn would dispatch to an exact-type implementation which would provide
any implicit conversions, and so-on.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could point out that this happens even when adding together (say) i32s.

To be precise, the expectation is that the SemIR for `x + y` should as a
consequence model all of:

- Looking up the `Core.AddWith` interface.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

Suggested change
- Looking up the `Core.AddWith` interface.
- Looking up the `AddWith` interface within the standard library package.

In particular, an unqualified lookup of the name Core isn't part of the design. (Performing a qualified lookup for the interface names wasn't my intent in #1083 either, but I don't think that's an important distinction, and I'd be OK with the rule being that you do perform that qualified lookup. But I'd be pretty strongly opposed to using an unqualified lookup for the name Core.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I understand the confusion when we've discussed this in the past.

Definitely not suggesting we should be doing arbitrary unqualified lookup. I agree that would be bad.

Really, this seems to suggest that there is a fundamental gap -- we should have some unambiguous way of looking up a name in the Core package without doing that name lookup. We have package.Name, we kind of need (package Core).Name but with some much better syntax. Is there a way I could frame this in pseudo syntax here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with the syntax you suggested, eg:

Suggested change
- Looking up the `Core.AddWith` interface.
- Looking up the `(package Core).AddWith` interface, where `package Core` is pseudo-syntax that directly names the `Core` package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a slightly different imagined syntax and clarified the behavior and that these are all imagined syntaxes.

shortcuts in the SemIR model for common cases with a option to disable them for
testing and debugging.

## Rationale
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I think would be worth discussing here is the impact of this proposal on cycles in the design: it'd be easy for us to accidentally define (for example) binding in terms of function calls and function calls in terms of bindings in such a way that we'd never bottom out. By requiring the toolchain to follow the design and not take shortcuts, at least under a flag, we can empirically test that our foundations actually work.

This means, for example, however we cut the loop of a function call desugaring into another function call needs to be determined in the design, not merely done in the toolchain at a convenient spot. So we shouldn't have the toolchain say "this is a direct call to a function; I'm not using the full function call machinery to dispatch it, because that would end up resulting in another function call", and instead we should have the design say that, if that's the intended approach. And if the intended approach is instead that we do use the full function call machinery, including impl lookup, but only once, then the design should say that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried adding a section about this. Not sure I really explained it well though, so ssuggestions welcome. =]

Copy link
Contributor Author

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for comments, most addressed, and one follow-up question inline!

That is, each use af `x + y` should turn into roughly equivalent SemIR as would
be used to model the rewritten semantics of `x.(Core.AddWith(typeof(y)).Op)(y)`.
This in turn would dispatch to an exact-type implementation which would provide
any implicit conversions, and so-on.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense, I think done.

To be precise, the expectation is that the SemIR for `x + y` should as a
consequence model all of:

- Looking up the `Core.AddWith` interface.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I understand the confusion when we've discussed this in the past.

Definitely not suggesting we should be doing arbitrary unqualified lookup. I agree that would be bad.

Really, this seems to suggest that there is a fundamental gap -- we should have some unambiguous way of looking up a name in the Core package without doing that name lookup. We have package.Name, we kind of need (package Core).Name but with some much better syntax. Is there a way I could frame this in pseudo syntax here?

shortcuts in the SemIR model for common cases with a option to disable them for
testing and debugging.

## Rationale
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried adding a section about this. Not sure I really explained it well though, so ssuggestions welcome. =]

chandlerc and others added 6 commits May 8, 2024 19:02
The toolchain's [Semantic IR][semir] should start off modeling the full,
complex, and rich library-based and generic extension point semantics of Carbon
without eliding any layers or rewrites for compile time efficiency. We shouldn't
front-load elision or optimization when implementing the designs.

Once we have a full-fidelity implementation, we should work to build an
efficient elision, short-circuit, or common case simplification into the design
itself sufficient to make the SemIR model efficient. Only if we cannot find a
reasonable approach for that should we diverge the SemIR model to optimize its
efficiency, and we should preserve full fidelity in an optional mode.

[semir]:
    https://docs.google.com/document/d/1RRYMm42osyqhI2LyjrjockYCutQ5dOf8Abu50kTrkX0/edit?resourcekey=0-kHyqOESbOHmzZphUbtLrTw#heading=h.503m6lfcnmui
@chandlerc
Copy link
Contributor Author

(Fixed weird mis-merge, sorry about that)

@zygoloid zygoloid mentioned this pull request May 11, 2024
@chandlerc chandlerc requested a review from zygoloid May 14, 2024 01:47
Copy link
Contributor Author

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL, I think the comments are addressed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants