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

Stable MonoItem #116465

Closed
wants to merge 1 commit into from
Closed

Conversation

ericmarkmartin
Copy link
Contributor

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2023

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 6, 2023
@ericmarkmartin
Copy link
Contributor Author

r? @celinval

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2023

Failed to set assignee to celinval: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@ericmarkmartin ericmarkmartin marked this pull request as ready for review October 6, 2023 01:46
@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

DropGlue(DefId, Option<Ty>),
CloneShim(DefId, Ty),
FnPtrAddrShim(DefId, Ty),
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should expose all the kinds of shims rustc has. This list changes occasionally. Instead I think you should have Item, Intrinsic, Virtual and a generic Shim variant which only exposes just enough to get the Stable MIR for the specific instantiation of said shim.

#[derive(Clone, Debug)]
pub struct Instance {
pub def: InstanceDef,
pub args: GenericArgs,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe merge this into the InstanceDef variants? For shims this duplicates information with the Ty and having it get out of sync could potentially crash the compiler and if it doesn't crash would likely misbehave.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 6, 2023

Can we avoid having MonoItem and Instance entirely?

cc @ouz-a this is the same stuff that I've been talking about in https://rust-lang.zulipchat.com/#narrow/stream/320896-project-stable-mir/topic/Normalization.20of.20generic.20ZST/near/395236860

Only that in your case we can avoid it, and I'm hoping we can avoid it here, too, by keeping the result of Instance::resolve within the rustc_smir crate and only using it to directly invoke instance_mir or similar functions.

@ericmarkmartin I'm assuming this is for your experiments in using SMIR in Kani? How does that logic there use Instance? Can we just use an Opaque in the same place for uniquely identifying an instance, but without any extra usable information in that identifier? It would mean you'd have to store a Body immediately, but that may be ok and sufficient

@bors
Copy link
Contributor

bors commented Oct 10, 2023

☔ The latest upstream changes (presumably #116605) made this pull request unmergeable. Please resolve the merge conflicts.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 17, 2023
Add MonoItems and Instance to stable_mir

Also add a few methods to instantiate instances and get an instance definition. We're still missing support to actually monomorphize the instance body.

This is related to rust-lang/project-stable-mir#36

r? `@oli-obk`

`@oli-obk` is that what you were thinking? I incorporated `@bjorn3` idea of just adding a Shim instance definition in rust-lang#116465.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 17, 2023
Add MonoItems and Instance to stable_mir

Also add a few methods to instantiate instances and get an instance definition. We're still missing support to actually monomorphize the instance body.

This is related to rust-lang/project-stable-mir#36

r? ``@oli-obk``

``@oli-obk`` is that what you were thinking? I incorporated ``@bjorn3`` idea of just adding a Shim instance definition in rust-lang#116465.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2023
Rollup merge of rust-lang#116719 - celinval:smir-mono, r=oli-obk

Add MonoItems and Instance to stable_mir

Also add a few methods to instantiate instances and get an instance definition. We're still missing support to actually monomorphize the instance body.

This is related to rust-lang/project-stable-mir#36

r? ``@oli-obk``

``@oli-obk`` is that what you were thinking? I incorporated ``@bjorn3`` idea of just adding a Shim instance definition in rust-lang#116465.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants