Preliminary support for MIR in trans #29217
Conversation
|
⛵ |
|
Note though that the code is intentionally a WIP :) |
| } | ||
| }; | ||
|
|
||
| let old_region_bounds_pairs_len = self.region_bound_pairs.len(); | ||
|
|
||
| // Collect the types from which we create inferred bounds. | ||
| // For the return type, if diverging, substitute `bool` just | ||
| // because it will have no effect. |
There was a problem hiding this comment.
Sounds something like what would get fixed by making diverging a first class type, no?
|
☔ The latest upstream changes (presumably #29254) made this pull request unmergeable. Please resolve the merge conflicts. |
| from_end: false, | ||
| min_length: _ } => { | ||
| let base_ty = tr_base.ty.to_ty(tcx); | ||
| let lloffset = common::C_u32(bcx.ccx(), offset); |
There was a problem hiding this comment.
Should this be using C_uint and ConstantIndex be adjusted to use a uint as well?
There was a problem hiding this comment.
Hmm, thinking about it, that would make the available values differ when cross-compiling. 32bit at least gives consistent results for now.
|
A few things ended up a commit early or late, LGTM otherwise and feels a lot cleaner than our current trans, though that's probably to be expected :-) I like the more thinner abstraction compared to I can't really comment on the debuginfo story either. While I know a bit about the DI types, I have no idea how the mapping to source locations work. r=me with the tidy error fixed |
|
I'd like to see rvalues translated in a way that doesn't always require a reified destination. We have a distinction in the MIR between real locations and temporaries, we should make use of it. Trying to deal with the issue of (almost) always needing a real destination causes enough problems in the current trans code that I'd hate to see the same mistake repeated here. Looking at the code, extending LValueRef to indicate by-value or by-ref, then passing that directly into trans_rvalue is probably enough. With the ValueRef being write-once in the by-value case. In the creation of the MirContext, the LValueRefs can then be created with the appropriate setting. Even if right now we just keep it as "always by-ref", it provides a good base for handling by-value-only lvalues in the future. Right now the code is set up to assume that LValueRefs are always allocas, which seems inconsistent with the idea that might want to avoid unnecessary allocas in the future. I'd like to see at least a basic handling of that case (i.e. by-value LValueRef) before this lands, even if it's peppered with |
|
On Mon, Oct 26, 2015 at 08:21:02PM -0700, James Miller wrote:
I was debating about the best way to handle this. I am inclined to My plan was to identify write-once temporaries whose address is never You're right that this will require isolating a path for processing Anyway, I can try to mock this up to show you what I mean. I'm not |
|
@nikomatsakis well either way I think basic "support" for it should be in the initial PR. Just having the delegation scheme in place should be enough to start. I just really don't want to end up with a massive amount of refactoring work because it was delayed until later and further changes didn't consider it. It'll be way easier to say "use this API instead" in a code review if said API actually exists. |
|
I have to agree with @Aatch, refactoring the existing trans code to support that optimization ended up reaching effectively all of trans and I have no idea how that could have been reviewed or merged. |
|
OK, seems reasonable, I'll take a stab at implementing what I had in mind. On Thu, Oct 29, 2015 at 3:17 AM, Eduard-Mihai Burtescu <
|
nikomatsakis
force-pushed
the
mir-trans
branch
from
64d4f3c
to
c4ed88a
Compare
| // | ||
| // Note that this is currently duplicated with src/libcore/ops.rs | ||
| // which does the same thing, and it would be nice to perhaps unify | ||
| // these two implementations on day! Also note that we call `fmod` |
There was a problem hiding this comment.
typo: "on day" -> "one day"
|
☔ The latest upstream changes (presumably #29486) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I just now saw the debuginfo-related things here. Debuginfo source locations in LLVM not only need the span but also the lexical scope of each source location. This information is later on used in the debugger to determine which variables are currently in scope. For this purpose we currently build a "scope map" for each function, which maps NodeIDs to the appropriate DI/DWARF scope descriptor. |
|
@michaelwoerister ok -- we do preserve scope information in MIR at the moment -- I envisioned this as temporary, but it seems like there are more and more reasons for it to stay around -- we should chat briefly about how best to connect those two pieces. |
nikomatsakis
force-pushed
the
mir-trans
branch
2 times, most recently
from
4ad49d7
to
98bd698
Compare
nikomatsakis
force-pushed
the
mir-trans
branch
from
98bd698
to
1efa4a8
Compare
| llarg | ||
| } else if common::type_is_fat_ptr(tcx, arg_ty) { | ||
| // we pass fat pointers as two words, but we want to | ||
| // represent them internally as a pointer two two words, |
|
Looks good. Analysing the MIR can be done at a later date, but now the code has to be able to handle non-alloca LValues, even if they don't come up right now. |
|
@Aatch hmm, I had a commit to that effect, I must have accidentally dropped the hunks from the patch. I'll maybe just add one that builds up the hashmap, it's not a lot more code, it'd be nice to see the system working. |
nikomatsakis
force-pushed
the
mir-trans
branch
from
1efa4a8
to
087c72d
Compare
nikomatsakis
force-pushed
the
mir-trans
branch
from
087c72d
to
d328072
Compare
|
@Aatch ok, see the latest pushes (I rebased, but I kept the commits separate so you could see what has changed). In particular d328072 adds the code you were referring to (handle references to temps that have no alloca), and 2b9233a actually skips allocas when possible. Under these changes, my sample test case produces the following IR: There is still a bit of indirection due to the named variables |
nikomatsakis
force-pushed
the
mir-trans
branch
from
7e6a79f
to
441273c
Compare
|
Ok, so I added enough new code that somebody ought to review it, I think. @dotdash are you up to reviewing those last four or five commits? |
|
I'll take a look this evening 2015-11-03 15:05 GMT+01:00 Niko Matsakis [email protected]:
|
|
The alloca avoidance looks like I had expected, so r=me with nits and that fat-pointer thing fixed. If you feel like it, it might be nice to check which codegen tests already survive being built through MIR and having them duplicated to cover both current trans and MIR trans, but I don't feel that's something that has to be done right now. |
@brson and I were discussing an alternative of possibly setting up some kind of "arewemiryet.com" infrastructure that would regularly test with MIR enabled and display which tests are failing with MIR. And once all tests work, it would test which crates are failing in the ecosystem. In any case, I think the support is as yet still a bit incomplete. |
nikomatsakis
force-pushed
the
mir-trans
branch
from
2c8340c
to
b46c0fc
Compare
|
@bors r=dotdash |
|
📌 Commit b46c0fc has been approved by |
|
⌛ Testing commit b46c0fc with merge 22d8bc5... |
|
💔 Test failed - auto-linux-64-x-android-t |
|
@bors r=dotdash |
|
📌 Commit e787863 has been approved by |
This branch implements a variant of trans that is based on MIR. It is very incomplete (intentionally), and had only the goal of laying out enough work to enable more incremental follow-on patches. Currently, only fns tagged with `#[rustc_mir]` use the new trans code. I plan to build up a meta-issue as well that tracks the various "not-yet-implemented" points. The only fn that has been tested so far is this amazingly complex "spike" fn:
```rust
#[rustc_mir]
fn sum(x: i32, y: i32) -> i32 {
x + y
}
```
In general, the most interesting commit is the last one. There are some points on which I would like feedback from @rust-lang/compiler:
- I did not use `Datum`. Originally, I thought that maybe just a `ValueRef` would be enough but I wound up with two very simple structures, `LvalueRef` and `OperandRef`, that just package up a `ValueRef` and a type. Because of MIR's structure, you don't wind up mixing by-ref and by-value so much, and I tend to think that a thinner abstraction layer is better here, but I'm not sure.
- Related to the above, I expect that sooner or later we will analyze temps (and maybe variables too) to find those whose address is never taken and which are word-sized and which perhaps meet a few other criteria. For those, we'll probably want to avoid the alloca, just because it means prettier code.
- I generally tried to re-use data structures from elsewhere in trans, though I'm sure we can trim these down.
- I didn't do any debuginfo primarily because it seems to want node-ids and we have only spans. I haven't really read into that code so I don't know what's going on there.
r? @nrc
|
❇️ |
This branch implements a variant of trans that is based on MIR. It is very incomplete (intentionally), and had only the goal of laying out enough work to enable more incremental follow-on patches. Currently, only fns tagged with
#[rustc_mir]use the new trans code. I plan to build up a meta-issue as well that tracks the various "not-yet-implemented" points. The only fn that has been tested so far is this amazingly complex "spike" fn:In general, the most interesting commit is the last one. There are some points on which I would like feedback from @rust-lang/compiler:
Datum. Originally, I thought that maybe just aValueRefwould be enough but I wound up with two very simple structures,LvalueRefandOperandRef, that just package up aValueRefand a type. Because of MIR's structure, you don't wind up mixing by-ref and by-value so much, and I tend to think that a thinner abstraction layer is better here, but I'm not sure.r? @nrc