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.

Sign up for GitHub

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

Jump to bottom

Add const-eval support for indirects #61437

Merged
merged 2 commits into from
Jun 4, 2019
Merged

Add const-eval support for indirects #61437

merged 2 commits into from
Jun 4, 2019

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Jun 1, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 1, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@Centril
Copy link
Contributor

Centril commented Jun 1, 2019

cc @oli-obk

Shouldn't this have some tests?

@pvdrz pvdrz changed the title Add const-eval support for inderects Add const-eval support for indirects Jun 2, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
oli-obk
oli-obk requested changes Jun 2, 2019
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Some tests would be great indeed. iirc @wesleywiser mentioned that promoteds will cause Indirects to occur. Not sure how to write a test around that, maybe something with casts of promoted values?

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Sign in to view
@wesleywiser

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2019

Unfortunately I don't understand what the current code is doing. By reading the code I would have assumed this to not work, but it does, so my understanding must be wrong.

What I thought is happening:

  1. we get an Operand::Indirect, which is kind of like a C++ reference, it refers to the value, but the background logic is a pointer. So It's not a ty::Ref in the type system, but a directly e.g. a ty::Int.
  2. read_immediate converts this into an Immediate (which can only be Immediate::Scalar or Immediate::ScalarPair), by actually reading from the backing memory.
  3. Then, operand_from_ref is called
  4. which takes a pointer out of the Immediate::Scalar (so the value, iff that value is a pointer)
  5. then creates a ByRef to wherever the pointer points to, but uses the ImmTy's layout

I assumed 5. to cause us to to essentially make this operation a deref operation, which should totally not work for casting.

Looking at read_immediate (we should probably use try_read_immediate to future proof this and not cause ICEs), I don't see how we could end up doing anything but reading from the MPlace...

wesleywiser
wesleywiser requested changes Jun 3, 2019
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

I think we should probably just squash all of these commits together. You can do that by running

git rebase -i origin/master

(assuming origin is the name of the https://github.com/rust-lang/rust repo).

Then in your editor pick the first commit and squash all the others.

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@pvdrz
Copy link
Contributor Author

pvdrz commented Jun 4, 2019

Ok the tests passed, I rebased and I'll wait until tomorrow for CI

wesleywiser
wesleywiser approved these changes Jun 4, 2019
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me but I'd like @oli-obk to sign off as well.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 4, 2019

@bors r=wesleywiser,oli-obk

@bors
Copy link
Contributor

bors commented Jun 4, 2019

📌 Commit b253678 has been approved by wesleywiser,oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2019
bors added a commit that referenced this pull request Jun 4, 2019
@bors
Auto merge of #61437 - christianpoveda:const-eval-indirects, r=wesley…
acda261 
…wiser,oli-obk

Add const-eval support for indirects

r? @wesleywiser
@bors
Copy link
Contributor

bors commented Jun 4, 2019

⌛ Testing commit b253678 with merge acda261...

@bors
Copy link
Contributor

bors commented Jun 4, 2019

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 4, 2019
@bors bors merged commit b253678 into rust-lang:master Jun 4, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #61437!

Tested on commit acda261.
Direct link to PR: #61437

🎉 rls on windows: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 4, 2019
@rust-highfive
📣 Toolstate changed by rust-lang/rust#61437!
816fdb7 
Tested on commit rust-lang/rust@acda261.
Direct link to PR: <rust-lang/rust#61437>

🎉 rls on windows: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
@pvdrz pvdrz deleted the const-eval-indirects branch June 4, 2019 16:47
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tesuji tesuji tesuji left review comments

@oli-obk oli-obk oli-obk requested changes

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pvdrz @rust-highfive @Centril @wesleywiser @oli-obk @bors @tesuji