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

Prepare ConstVal for constant propagators and reduce `eval_const_expr` in MIR #33274

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
4 participants
@oli-obk
Copy link
Contributor

oli-obk commented Apr 29, 2016

rebased eager const eval over @eddyb 's MIR-constants PR #33130

fixes #29928
fixes #33252

  1. MIR creation now only calls eval_const_expr on literals and negated literals
    • previously all const/static definitions were attempted to be translated, but could fail due to the fact that eval_const_expr can't eval all kinds of constants
    • since "unevaluable" constants are translated from MIR now, there was no reason to keep evaluating constants that aren't needed for array lengths or patterns
    • a next step would be to convert literals manually to MIR constants instead of using eval_const_expr_partial
  2. ConstVal contains owned versions of struct fields, tuple fields, array elements and the repeat value
  3. drive-by fixes for #33252, #33031, #31384 + tests

r? @eddyb

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 29, 2016

@oli-obk I disagree with putting MIR constant aggregates in ConstVal.
Constant propagation on MIR can use special structures to express arbitrary knowledge beyond what constant evaluation needs to understand, and I think it's better to just have a cheap ConstVal that can be passed around (in miri I believe this is PrimVal, @tsion correct me if I'm wrong).

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Apr 29, 2016

I'm not sure I understand. ConstVal is pretty cheap to pass around. It went from 32 bytes to 48 in my changes, and once we can merge the Struct variant into the Tuple by removing the names, it'll be back to 32 bytes. And the size isn't really relevant as it's never cloned. (for reference: PrimVal has 24 bytes).

We can refactor ConstVal into multiple Literal variants in MIR. One for PrimVal and one for aggregates, but we can do that when there's need. Right now we already have a structure that's holding our constants.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 29, 2016

@oli-obk Yes, but you can't copy the ConstVal around which is more useful IMO than representing MIR rvalues in a redundant way.
I don't think we need aggregate literals in the MIR.
They're rvalues because that keeps things simple, and again, Vec<ConstVal> isn't going to help constant propagation, which would need to use something like Vec<MaybeConstant>.

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Apr 29, 2016

I don't think we need aggregate literals in the MIR. They're rvalues because that keeps things simple,

When I suggested that on #rust-internals, I was told that we should end up merging aggregates that consist solely of constants into a constant aggregate.

and again, Vec isn't going to help constant propagation, which would need to use something like Vec.

Ah, we're talking about different structures. The fact that constprop exists should not change the MIR. The MIR should represent the code as is. All constant propagation can do is manipulate that code.

The constant propagation pass can have internal datastructures that store additional info about certain temp/var decls. This way the const prop pass can propagate more than constants (it can do move-skips like a -> b -> c to a -> c where a is not a constant)

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 29, 2016

@oli-obk Ah, do you have a link to that discussion?
Maybe they were onto something I'm not immediately seeing.
Either way, I want more opinions on this before we proceed.
cc @rust-lang/compiler @nagisa

@oli-obk

This comment has been minimized.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 2, 2016

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

@oli-obk oli-obk force-pushed the oli-obk:eager_const branch from 066de64 to 37c3beb May 9, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 11, 2016

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

oli-obk added some commits Apr 29, 2016

don't const eval constants during MIR creation
this didn't work very well anyway, because const_eval can't eval all kinds of constants.

@oli-obk oli-obk force-pushed the oli-obk:eager_const branch from 37c3beb to 6ac3812 May 11, 2016

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented May 11, 2016

@eddyb any update on the design decision?

+rebased

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented May 11, 2016

@rust-lang/compiler, do we want to discuss this PR today?
I'm worried about recursive ConstVal since I'd only use ConstVal for leaves in the MIR.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 12, 2016

@eddyb

do we want to discuss this PR today?

let's discuss tomorrow, along with a bit more about constants in general

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented May 19, 2016

Did the discussion yield any results wrt aggregate constants?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 23, 2016

We never did talk about it :(

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 5, 2016

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

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Jul 5, 2016

since my original notion was to not have aggregates in MIR, and @eddyb feels that way, too, I'll close this.

@oli-obk oli-obk closed this Jul 5, 2016

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.