Move collector to librustc_mir::monomorphize #45525

MaikKlein · 2017-10-25T16:08:01Z

I moved the collector to rustc_mir
I renamed TransItem to MonoItem. (I still need to fix up comments and variable names)
I got rid of common.rs and monomorphize.rs from librustc_trans_utils. I moved most of the functionality into TyCtxt. I realized that the librustc_trans_utils::common.rs was just copy pasted from librustc_trans::common.rs.

Should I also get rid of the librustc_trans::common.rs in this PR? Most of the functionality seems a bit useless, I decided to put some of it into TyCtxt but maybe that is not the correct action here.

Should I also get rid of librustc_trans_utils completely here? Or should I do it in a separate PR?

rust-highfive · 2017-10-25T16:08:24Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eddyb · 2017-10-25T23:11:16Z

r? @eddyb

eddyb · 2017-10-26T12:09:46Z

src/librustc/ty/instance.rs

@@ -45,6 +45,51 @@ pub enum InstanceDef<'tcx> {
    CloneShim(DefId, Ty<'tcx>),
 }

+impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {
+    pub fn is_inline_instance(


Where is this used? In general, I'd expect these methods to be on Instance, or where possible, InstanceDef, and private if only used locally. (I was trying to come up with better name for them)

This is only used in requests_inline afaik. But the original function is used in callee.rs and trans_item_rs. I can move it to instance, I wasn't sure where to put it because it takes an instance and a tyctxt.

eddyb · 2017-10-26T12:10:32Z

src/librustc/ty/instance.rs

+        }
+    }
+
+    pub fn requests_inline(


I think this should be called requires_local_instance or something else along those lines.

eddyb · 2017-10-26T12:12:03Z

src/librustc/ty/mod.rs

@@ -2380,6 +2380,21 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
    }
 }

+impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {
+    pub fn is_sized(self, ty: Ty<'tcx>) -> bool {


Ideally these two methods would be avoided. I wonder what the DUMMY_SP is used for - is_sized should never cause an ICE or an user error.

I wasn't sure about those functions, there are still a bunch of them in rustc_trans::common.rs. Initially I thought I would move them into TyCtxt but then I asked myself if they should even exist. Should I remove them?

Depends on what the usage looks like.

I haven't investigated it so far. These functions were just copied (not moved) into trans_utils::common.rs and therefore are only used in the collector IIRC. But the original functions are probably more widespread.

Replacing the uses in the collector with the more common forms is probably the best first step.

eddyb · 2017-10-26T12:12:21Z

src/librustc_mir/monomorphize/collector.rs

-//!
-//!
-//! General Algorithm
+//! - Methm


This looks to be accidentally truncated.

Not sure what happened, I am fixing it now.

eddyb · 2017-10-26T12:13:27Z

src/librustc_trans/Cargo.toml

@@ -27,6 +27,7 @@ rustc_incremental = { path = "../librustc_incremental" }
 rustc_llvm = { path = "../librustc_llvm" }
 rustc_platform_intrinsics = { path = "../librustc_platform_intrinsics" }
 rustc_trans_utils = { path = "../librustc_trans_utils" }


What's left of rustc_trans_utils?

https://github.com/MaikKlein/rust/tree/collector/src/librustc_trans_utils I am not sure where I should move the rest.

Oh I think you can throw them into librustc/middle if you want to move them. But nothing I see there should retain its current form anyway.

eddyb · 2017-10-27T09:43:01Z

src/librustc/ty/instance.rs

    ) -> bool {
        use hir::map::DefPathData;
-        let def_id = match instance.def {
+        let def_id = match self.def {


With only self.def being accessed, doesn't this method belong on InstanceDef? Same for requires_local_instance.

You are right, I haven't looked too closely.

eddyb · 2017-10-28T15:45:27Z

src/librustc/ty/instance.rs

@@ -45,6 +45,16 @@ pub enum InstanceDef<'tcx> {
    CloneShim(DefId, Ty<'tcx>),
 }

+impl<'a, 'tcx> Instance<'tcx> {
+    pub fn instance_ty(&self,


I'd call this ty. One thing I'm worried about is trans_apply_param_substs failing if this is accidentally used somewhere where a ParamEnv is needed or some such.
Shouldn't self.def.def_ty(tcx).subst(tcx, self.substs) suffice?

@eddyb I don't really know the details of substitutions yet. I have updated this function to use self.def.def_ty(tcx).subst(tcx, self.substs) but then I get an ICE in run_cargo (see latest build error). Everything seems to work when I use trans_apply_param_substs.

Should I still avoid trans_apply_param_substs? I can investigate further why it crashes if you want me to.

eddyb · 2017-10-28T15:46:32Z

src/librustc/ty/mod.rs

@@ -2380,6 +2380,17 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
    }
 }

+impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {
+    /// Given a DefId and some Substs, produces the monomorphic item type.


This shouldn't exist - maybe inline the body or use Instance::new(def_id, substs).ty(tcx) instead?

Oh sorry I should have looked closer, I am removing it now.

bors · 2017-10-28T16:21:01Z

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

bors · 2017-10-29T04:54:58Z

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

arielb1 · 2017-12-18T15:43:16Z

@bors r=eddyb

bors · 2017-12-18T15:43:17Z

📌 Commit effd99f has been approved by eddyb

arielb1 · 2017-12-18T16:12:43Z

@bors r=eddyb

bors · 2017-12-18T16:12:44Z

📌 Commit b6a7688 has been approved by eddyb

bors · 2017-12-18T16:33:56Z

⌛ Testing commit b6a768861d44c0c24be3156713cdbe04428406fd with merge b36b12eed2daecdf436755834f9e858141d212eb...

bors · 2017-12-18T16:48:34Z

💔 Test failed - status-travis

bjorn3 · 2017-12-18T17:01:45Z

[...]
[00:04:28] Command failed. Attempt 5/5:
[00:04:29] Cleared directory 'src/tools/miri'
[00:04:29] Submodule 'src/tools/miri' (https://github.com/solson/miri.git) unregistered for path 'src/tools/miri'
[00:04:29] Submodule 'src/tools/miri' (https://github.com/solson/miri.git) registered for path 'src/tools/miri'
[00:04:29] error: no such remote ref a23e587dc3a55a766ddd95454fd8368988cc7a3a
[00:04:29] Fetched in submodule path 'src/tools/miri', but it did not contain a23e587dc3a55a766ddd95454fd8368988cc7a3a. Direct fetching of that commit failed.
[00:04:29] The command has failed after 5 attempts.
[...]

eddyb · 2017-12-18T17:05:49Z

Need to undo the miri submodule change.

arielb1 · 2017-12-18T17:10:20Z

@bors r=eddyb

bors · 2017-12-18T17:10:21Z

📌 Commit 6e78b66 has been approved by eddyb

bors · 2017-12-18T19:33:47Z

⌛ Testing commit 6e78b66 with merge 0a440e76bff84933783d590cf47ee811c5b345a0...

bors · 2017-12-18T20:44:14Z

💔 Test failed - status-travis

kennytm · 2017-12-18T20:47:55Z

@bors retry #46313.

bors · 2017-12-18T23:33:21Z

⌛ Testing commit 6e78b66 with merge 68e6abacfa269e101e7f6f2e7f83d0527012ae0a...

bors · 2017-12-19T00:33:24Z

💔 Test failed - status-appveyor

bors · 2017-12-19T01:34:56Z

⌛ Testing commit 6e78b66 with merge b76f224...

Move collector to librustc_mir::monomorphize cc #44334 and #45276 * I moved the collector to rustc_mir * I renamed `TransItem` to `MonoItem`. _(I still need to fix up comments and variable names)_ * I got rid of `common.rs` and `monomorphize.rs` from `librustc_trans_utils`. I moved most of the functionality into `TyCtxt`. I realized that the `librustc_trans_utils::common.rs` was just copy pasted from `librustc_trans::common.rs`. Should I also get rid of the `librustc_trans::common.rs` in this PR? Most of the functionality seems a bit useless, I decided to put some of it into `TyCtxt` but maybe that is not the correct action here. Should I also get rid of `librustc_trans_utils` completely here? Or should I do it in a separate PR?

bors · 2017-12-19T04:20:55Z

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

rust-highfive assigned pnkfelix Oct 25, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2017

rust-highfive assigned eddyb and unassigned pnkfelix Oct 25, 2017

MaikKlein force-pushed the collector branch from 31f0a2d to 70d2881 Compare October 26, 2017 09:32

eddyb reviewed Oct 26, 2017

View reviewed changes

MaikKlein force-pushed the collector branch from acd437f to 74b39b6 Compare October 26, 2017 13:01

eddyb reviewed Oct 27, 2017

View reviewed changes

MaikKlein force-pushed the collector branch 3 times, most recently from f8b8e0c to af37af2 Compare October 27, 2017 12:53

eddyb reviewed Oct 28, 2017

View reviewed changes

MaikKlein force-pushed the collector branch 2 times, most recently from 1bed931 to e0bcb86 Compare October 28, 2017 19:09

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 29, 2017

MaikKlein force-pushed the collector branch 5 times, most recently from ec4b1dd to d13f021 Compare October 29, 2017 22:48

MaikKlein added 2 commits December 18, 2017 19:10

Fix incorrect rebase in collector::find_vtable_types

c847cf3

Add rustc_data_structures for trans_utils/lib.rs

6e78b66

arielb1 force-pushed the collector branch from b6a7688 to 6e78b66 Compare December 18, 2017 17:10

bors merged commit 6e78b66 into rust-lang:master Dec 19, 2017

This was referenced Dec 19, 2017

WIP: Parallelize passes using rayon #46564

Closed

Implements RFC 1937: ? in main #46479

Merged

Remove impl Foo for .. {} in favor auto trait Foo {} #46480

Closed

Prevent unwinding past FFI boundaries #46833

Merged

MaikKlein mentioned this pull request Dec 19, 2017

Collector in trans is private #44334

Closed

Move collector to librustc_mir::monomorphize #45525

Move collector to librustc_mir::monomorphize #45525

Conversation

MaikKlein commented Oct 25, 2017

rust-highfive commented Oct 25, 2017

eddyb commented Oct 25, 2017

Choose a reason for hiding this comment

MaikKlein Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaikKlein Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 28, 2017

bors commented Oct 29, 2017

arielb1 commented Dec 18, 2017

bors commented Dec 18, 2017

arielb1 commented Dec 18, 2017

bors commented Dec 18, 2017

bors commented Dec 18, 2017

bors commented Dec 18, 2017

bjorn3 commented Dec 18, 2017

eddyb commented Dec 18, 2017

arielb1 commented Dec 18, 2017

bors commented Dec 18, 2017

bors commented Dec 18, 2017

bors commented Dec 18, 2017

kennytm commented Dec 18, 2017

bors commented Dec 18, 2017

bors commented Dec 19, 2017

bors commented Dec 19, 2017

bors commented Dec 19, 2017

MaikKlein Oct 26, 2017 •

edited

Loading

MaikKlein Oct 27, 2017 •

edited

Loading