Implement more TIR data structures. #3
Conversation
ykpack/src/types.rs
Outdated
| @@ -93,7 +96,16 @@ impl Display for BasicBlock { | |||
| pub enum Statement { | |||
| Nop, | |||
| Assign(Place, Rvalue), | |||
| Unimplemented, // FIXME | |||
| SetDiscriminant{place: Place, variant_index: VariantIndex}, | |||
There was a problem hiding this comment.
To fit in with the other members of the enum, should this be SetDiscriminant(Place, VariantIndex)?
There was a problem hiding this comment.
Yeah, I've been pondering on this myself. In MIR it's a random mix.
Ideally I'd use only tuple-like variants, but I know I'd be upset that I'd have to break my rule if we had a variant with lots of the same type.
E.g.:
enum Taps {
...
Mixer(usize, usize, usize, usize),
}
Here a struct-like would be preferred, so that we can name the fields.
Thoughts?
There was a problem hiding this comment.
I agree that when you get repetitions of the same type you want to use names, but that's not the case here. [I also sometimes use names when the type doesn't give much away about what the "thing" is.] Up to you.
| /// Describes a projection operation upon a projection base. | ||
| #[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone)] | ||
| pub enum ProjectionElem<V> { | ||
| Unimplemented(PhantomData<V>), // FIXME |
There was a problem hiding this comment.
There's supposed to be another structure in there, but I don't want to implement it just yet.
The missing struct is parameterised by V.
|
I think this is ready for re-review. |
|
Please squash. |
|
My bad, the tests were broken, and I also detected rogue tabs. Still OK to squash? |
|
The new commits look fine. Please squash. |
|
Squashed. Left the tab fixes separate, because they applied to different lines to what was touched before. |
|
You can try using |
|
Yeah, it's on my to do list to look at. |
|
You can try: then manually check what it's done before doing: I think it's worth trying before we merge this. If it's a disaster, we can merge as-is. |
|
I'll give it a go, but I expect it to be a no-op, since I already squashed. |
|
If those tab changes happened in this PR, it is li-kely do the right thing. [Sometimes it doesn't, but not often.] |
|
As expected, it's a no-op. Let's r+? |
|
Wait, it does something I dont understand. Can we merge and I'll look at this another day. |
|
Ah, I might have missed this:
I think you're saying "the tabs fix is for stuff that wasn't changed in this PR"? If so, yes, git can't work for this case. |
|
The tab stuff was in this PR, but it was already in a separate commit how I desired it :P |
|
What I meant was: I think you're changing tabs to spaces in code that wasn't touched by the first commit. [Indeed, I've just checked and this is the case.] bors r+ |
3: Implement more TIR data structures. r=ltratt a=vext01 This is a companion commit to: softdevteam/ykrustc#18 Co-authored-by: Edd Barrett <[email protected]>
Build failed |
|
Can I squash that? |
|
Please squash. |
This is a companion commit to: softdevteam/ykrustc#18
|
splat |
|
bors r+ |
3: Implement more TIR data structures. r=ltratt a=vext01 This is a companion commit to: softdevteam/ykrustc#18 Co-authored-by: Edd Barrett <[email protected]>
Build succeeded |
18: Implement more TIR lowerings. r=ltratt a=vext01 This is a whole load more TIR lowerings. The TIR graphs are starting to look more complete, although there's still a lot more to do. I only stopped here because the changes are getting large, and more edits can come in later PRs. There's one part of this code (which I will point out), which i'm likely to change. It involves a type-parameterised MIR struct, which I think ultimately we will make concrete in TIR. However, I want to see the outcome of this upstream PR first: rust-lang/rust#60441 There's a ykpack change to accompany this: ykjit/yk#3 Co-authored-by: Edd Barrett <[email protected]>
18: Implement more TIR lowerings. r=ltratt a=vext01 This is a whole load more TIR lowerings. The TIR graphs are starting to look more complete, although there's still a lot more to do. I only stopped here because the changes are getting large, and more edits can come in later PRs. There's one part of this code (which I will point out), which i'm likely to change. It involves a type-parameterised MIR struct, which I think ultimately we will make concrete in TIR. However, I want to see the outcome of this upstream PR first: rust-lang/rust#60441 There's a ykpack change to accompany this: ykjit/yk#3 Co-authored-by: Edd Barrett <[email protected]>
3: Add a link to the hosted docs. r=ltratt a=vext01 Add a link to the hosted docs. Co-authored-by: Edd Barrett <[email protected]>
Previously `load` and `store` both took SSA pointer variables directly.
This commit adds an offset relative to those variables. In other words,
we can now turn:
```
%7: ptr = ...
%8: ptr = ptr_add %7, 8
%9: i8 = load %8
```
into:
```
%7: ptr = ...
%9: i8 = load [%7 + 8]
```
This is effective on x64 (and should be on, at least, Arm too) because
"load from register + offset" can generally be encoded in a single
instruction.
We previously had a hack for the specific case above iff `ptr_add` was
immediately followed by `load/`store`. This commit generalises that:
the optimisation pass "inlines" `ptr_add` no matter how far back in the
trace it is. So this:
```
%7: ptr = ...
%8: ptr = ptr_add %7, 8
...
%5432: i8 = load %8
```
generates the same, efficient, code as if there was nothing between the
`ptr_add` and the `load`. We do the same optimisation for `store`s too.
This gives a 2.5% speed-up for big_loop.
There are, however, downsides:
1. We complicate the JIT IR syntax with `[%val + off]`. Currently I
haven't added JIT IR parser support for this, because it doesn't
seem very useful, but you can observe it when traces are printed.
2. We can no longer fit instructions in a single machine word: there
isn't space in `StoreInst` (and only 7 bits free in `LoadInst`).
TBH, I now think -- and this is entirely on my head! -- that
we were going to have to break this constrain at some point soon,
in order to allow bigger `InstIdx`s. Especially because we compile
in a thread, I think the practical effect of this is low (I can
still observe a speedup after all!), and if we had to go above 2
machine words, I'm not sure it would make much difference.
3. Philosophically speaking, we're extending the JIT IR simply to
express an optimisation. Frankly speaking, I think the right
long-term thing to do is -- probably -- to introduce another IR
that has these details in, but I have no desire to introduce
such a complication now.
4. This doesn't help quite as much as one would hope because a
surprising amount of `PtrAdd`s are only left around because they're
needed by a guard. Optimising this is a job for later.
I have -- and obviously this isn't enforceable! -- hacked around the
consequences of ykjit#3 by only allowing the optimiser to set `off` to a
non-zero value. That means most of the pipeline can avoid thinking about
this, but we shouldn't kid ourselves that this means there is no
additional complexity from this change. There is, the complexity is
permanent --- though, in my judgement, it is probably worthwhile. I also
doubt this is the last change along these lines (e.g. should we try
handling `DynPtrAdd`).
This is a companion commit to:
softdevteam/ykrustc#18