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

Translate shims using MIR #39628

Merged
merged 11 commits into from
Mar 20, 2017
Merged

Translate shims using MIR #39628

merged 11 commits into from
Mar 20, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Feb 7, 2017

This removes one large remaining part of old trans.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@bors
Copy link
Contributor

bors commented Feb 9, 2017

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

@KalitaAlexey
Copy link
Contributor

Hi @arielb1,
Will you describe your changes?
I am interested in what you are doing.

@arielb1
Copy link
Contributor Author

arielb1 commented Feb 9, 2017

@KalitaAlexey

Some functions in Rust, such as enum variant constructions (e.g. Option::<u32>::Some) and some trait methods (e.g. <fn(u32) as Fn(u32)>::call) are implemented through "shims". I am translating these shims more like normal functions.

@eddyb
Copy link
Member

eddyb commented Feb 13, 2017

cc @rust-lang/compiler

@mrhota
Copy link
Contributor

mrhota commented Feb 15, 2017

@arielb1 to what end? what you're doing and why you're doing it are equally important. Your commit messages don't provide information on your rationale 🙁

@jonas-schievink
Copy link
Contributor

@mrhota less special-casing and less convoluted code in the compiler

@solson
Copy link
Member

solson commented Feb 15, 2017

It also pulls trans-specific code out into MIR in a way that will work for alternative backends (e.g. miri, mir2wasm), reducing duplication between them.

#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
pub enum InstanceDef<'tcx> {
Item(DefId),
// <fn() as FnTrait>::call_*
Copy link
Contributor

Choose a reason for hiding this comment

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

A more explicit comment would be nice -- It's not clear to me what the Ty<'tcx> represents here.

@bors
Copy link
Contributor

bors commented Feb 25, 2017

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

@arielb1 arielb1 force-pushed the shimmir branch 2 times, most recently from f52b583 to 6f607d6 Compare March 7, 2017 23:42
Deep
}

pub trait DropElaborator<'a, 'tcx: 'a> : fmt::Debug {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this file not in librustc_mir/transform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's not a MIR pass.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

LGTM so far! r=me with those extra FIXMEs if you want to land it like this

pub shim_mir_cache: RefCell<FxHashMap<ty::InstanceDef<'tcx>,
&'tcx RefCell<Mir<'tcx>>>>,
pub mir_shimmer: RefCell<Option<Box<ty::MirShimmer<'tcx>>>>,

Copy link
Member

Choose a reason for hiding this comment

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

Can you put a FIXME on these to eventually integrate them better with the rest of on-demand?

@@ -2302,6 +2312,22 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
queries::mir::get(self, DUMMY_SP, did).borrow()
}

/// Return the possibly-auto-generated MIR of a (DefId, Subst) pair.
pub fn instance_mir(self, instance: ty::InstanceDef<'gcx>)
Copy link
Member

Choose a reason for hiding this comment

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

Same FIXME here, I suppose.

@@ -423,6 +423,8 @@ pub struct GlobalCtxt<'tcx> {
pub hir: hir_map::Map<'tcx>,
pub maps: maps::Maps<'tcx>,

// This is not a ty::maps cache because it contains Tys, instead
// it is structured similarly to selection_cache.
Copy link
Member

Choose a reason for hiding this comment

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

But if you have a dep_node method, you can use it to move this to on-demand.

@arielb1 arielb1 force-pushed the shimmir branch 2 times, most recently from 7d069ec to bd92573 Compare March 9, 2017 22:27
@bors
Copy link
Contributor

bors commented Mar 11, 2017

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

@arielb1 arielb1 changed the title [WIP] translate shims using MIR Translate shims using MIR Mar 15, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Mar 15, 2017

Now ready (except for cleanup, and I prefer to land it now given that this PR is rather big enough already). r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned pnkfelix Mar 15, 2017
@arielb1 arielb1 force-pushed the shimmir branch 3 times, most recently from 040f333 to 9f5def1 Compare March 16, 2017 12:12
@eddyb
Copy link
Member

eddyb commented Mar 19, 2017

I can't see the end of that appveyor log.

@arielb1
Copy link
Contributor Author

arielb1 commented Mar 19, 2017

sccache failure

@bors retry

@bors
Copy link
Contributor

bors commented Mar 20, 2017

⌛ Testing commit 5dc8548 with merge 35617fc...

@bors
Copy link
Contributor

bors commented Mar 20, 2017

💔 Test failed - status-appveyor

@arielb1
Copy link
Contributor Author

arielb1 commented Mar 20, 2017

@bors
Copy link
Contributor

bors commented Mar 20, 2017

⌛ Testing commit 5dc8548 with merge 0b1ddef...

@arielb1
Copy link
Contributor Author

arielb1 commented Mar 20, 2017

Mac builders offline

@bors retry

@bors
Copy link
Contributor

bors commented Mar 20, 2017

⌛ Testing commit 5dc8548 with merge 5e30f81...

@bors
Copy link
Contributor

bors commented Mar 20, 2017

💔 Test failed - status-travis

@arielb1
Copy link
Contributor Author

arielb1 commented Mar 20, 2017

@bors
Copy link
Contributor

bors commented Mar 20, 2017

⌛ Testing commit 5dc8548 with merge 134c4a0...

bors added a commit that referenced this pull request Mar 20, 2017
Translate shims using MIR

This removes one large remaining part of old trans.
@arielb1
Copy link
Contributor Author

arielb1 commented Mar 20, 2017

Mac test timeout?

@bors retry

@bors
Copy link
Contributor

bors commented Mar 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 134c4a0 to master...

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.

10 participants