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

Make place projections concrete. #60441

Merged
merged 1 commit into from May 25, 2019

Conversation

Projects
None yet
7 participants
@vext01
Copy link
Contributor

commented May 1, 2019

I'm not sure if we want this. I'm raising the PR for discussion

Whilst doing some work on our Rust fork, I noticed the following:

Once upon a time (commit 9bd35c0) there were two kinds of
projection: one for places, and one for constants. It therefore made
sense to share the Projection struct for both. Although the different
use-cases used different concrete types, sharing was made possible by
type-parameterisation of Projection.

Since then, however, the usage of projections in constants has
disappeared, meaning that (forgetting lifetimes for a moment) the
parameterised type is only every instantiated under one guise. So it may
as well be a concrete type. Right?

What do people think? This is entirely untested, although it does check.

If we don't want this, then we should at least update the incorrect comment against Projection.

Thanks

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

r? @eddyb

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

Show resolved Hide resolved src/librustc/mir/mod.rs Outdated

@vext01 vext01 referenced this pull request May 1, 2019

Merged

Implement more TIR lowerings. #18

@eddyb

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I thought this was because the MIR borrowck needs to replace the indexing values with an "abstract value" (i.e. treating a[i] and a[j] as potentially-the-same).

cc @pnkfelix @matthewjasper @nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I think that capability wound up not being used

@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

There's

pub type AbstractElem = ProjectionElem<AbstractOperand, AbstractType>;

and
pub type ProjectionKind = ProjectionElem<(), ()>;

But both of those only use ProjectionElem.

This seems fine, but Projection will eventually be refactored away in the Place 2.0 work.

@@ -1937,16 +1937,13 @@ impl_stable_hash_for!(struct Static<'tcx> {
kind
});

/// The `Projection` data structure defines things of the form `B.x`
/// or `*B` or `B[index]`. Note that it is parameterized because it is
/// shared between `Constant` and `Place`. See the aliases

This comment has been minimized.

Copy link
@eddyb

eddyb May 1, 2019

Member

Oh, wow, this comment has been wrong for most of MIR's existence, I think?
For reasons, Projection<...> isn't used by MIR borrowck, but ProjectionElem is (as AbstractElem).

I think that's why Projection remained parameterized as well, but I'm not sure.

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

OK, so is there a concensus on this? If you'd like this to go in, then I'll run some tests and check for fallout.

Projection will eventually be refactored away in the Place 2.0 work.

Is there some reading material on this please. This is likely to influence what I'm doing in my fork ;)

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

./x.py test --stage 1 src/test/{ui,compile-fail,run-pass} passes on my machine.

@vext01 vext01 marked this pull request as ready for review May 3, 2019

Show resolved Hide resolved src/librustc/mir/mod.rs Outdated
@bors

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

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

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Folks, before I go down the path of resolving those conflicts, do you want this change?

@eddyb

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Yes, sorry for not being clearer. This is an accidental leftover that everyone forgot about because of the ProjectionElem parametrization.

bors bot added a commit to softdevteam/ykrustc that referenced this pull request May 6, 2019

Merge #18
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:
softdevteam/yk#3

Co-authored-by: Edd Barrett <vext01@gmail.com>

bors bot added a commit to softdevteam/ykrustc that referenced this pull request May 7, 2019

Merge #18
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:
softdevteam/yk#3

Co-authored-by: Edd Barrett <vext01@gmail.com>
@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

synced. OK for me to squash?

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I think we should prefer the name Projection.

Oops, I missed this. Leave it with me.

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

That last commit does the renaming. Quite a lot of churn.

OK to squash?

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

lgtm now, squashing would be good, yes

Show resolved Hide resolved src/librustc/mir/mod.rs Outdated
@eddyb

eddyb approved these changes May 13, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

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

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

If everyone is OK with a4ba4fa, I'll squash.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0ac0be40:start=1558690454608116148,finish=1558690540971523343,duration=86363407195
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:01:09] 
######################################################################## 100.0%
[00:01:09] extracting /checkout/obj/build/cache/2019-04-11/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:01:09]     Updating crates.io index
[00:01:27] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:27] Build completed unsuccessfully in 0:00:31
[00:01:27] make: *** [prepare] Error 1
[00:01:27] Makefile:69: recipe for target 'prepare' failed
[00:01:28] Command failed. Attempt 2/5:
[00:01:28] Command failed. Attempt 2/5:
[00:01:29] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:29] Build completed unsuccessfully in 0:00:00
[00:01:29] make: *** [prepare] Error 1
[00:01:29] Makefile:69: recipe for target 'prepare' failed
[00:01:31] Command failed. Attempt 3/5:
[00:01:31] Command failed. Attempt 3/5:
[00:01:31] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:31] Build completed unsuccessfully in 0:00:00
[00:01:31] Makefile:69: recipe for target 'prepare' failed
[00:01:31] make: *** [prepare] Error 1
[00:01:34] Command failed. Attempt 4/5:
[00:01:34] Command failed. Attempt 4/5:
[00:01:35] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:35] Build completed unsuccessfully in 0:00:00
[00:01:35] make: *** [prepare] Error 1
[00:01:35] Makefile:69: recipe for target 'prepare' failed
[00:01:39] Command failed. Attempt 5/5:
[00:01:39] Command failed. Attempt 5/5:
[00:01:39] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:39] Build completed unsuccessfully in 0:00:00
[00:01:39] Makefile:69: recipe for target 'prepare' failed
[00:01:39] make: *** [prepare] Error 1
[00:01:39] The command has failed after 5 attempts.
---
travis_time:end:019a811a:start=1558690650708242223,finish=1558690650714068415,duration=5826192
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:04cf2ea0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:08ab9816
travis_time:start:08ab9816
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1e673699
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Sorry, I screwed up the last commit.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@vext01 yes, this is looking good now. So a rebase + squash is all that is necessary to get it merged

r? @oli-obk

Make place projections concrete.
Once upon a time (commit 9bd35c0) there were two kinds of
projection: one for places, and one for constants. It therefore made
sense to share the `Projection` struct for both. Although the different
use-cases used different concrete types, sharing was made possible by
type-parameterisation of `Projection`.

Since then, however, the usage of projections in constants has
disappeared, meaning that (forgetting lifetimes for a moment) the
parameterised type is only every instantiated under one guise. So it may
as well be a concrete type.

@vext01 vext01 force-pushed the vext01:try-to-kill-projection-params branch from 1bbff00 to 123a456 May 24, 2019

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Squashed.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

📌 Commit 123a456 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

⌛️ Testing commit 123a456 with merge 5245803...

bors added a commit that referenced this pull request May 25, 2019

Auto merge of #60441 - vext01:try-to-kill-projection-params, r=oli-obk
Make place projections concrete.

**I'm not sure if we want this. I'm raising the PR  for discussion**

Whilst doing some work on our Rust fork, I noticed the following:

Once upon a time (commit 9bd35c0) there were two kinds of
projection: one for places, and one for constants. It therefore made
sense to share the `Projection` struct for both. Although the different
use-cases used different concrete types, sharing was made possible by
type-parameterisation of `Projection`.

Since then, however, the usage of projections in constants has
disappeared, meaning that (forgetting lifetimes for a moment) the
parameterised type is only every instantiated under one guise. So it may
as well be a concrete type. Right?

What do people think? This is entirely untested, although it does check.

If we *don't* want this, then we should at least update the incorrect comment against `Projection`.

Thanks
@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 5245803 to master...

@bors bors added the merged-by-bors label May 25, 2019

@bors bors merged commit 123a456 into rust-lang:master May 25, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.