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

interpret: move nullary-op evaluation into operator.rs #128697

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 5, 2024

We call it an operator, so we might as well treat it like one. :)

Also use more consistent naming for the "evaluate intrinsic" functions. "emulate" is really the wrong term, this is a genuine implementation of the intrinsic semantics after all.

@rustbot
Copy link
Collaborator

rustbot commented Aug 5, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 5, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

self.tcx.offset_of_subfield(self.param_env, layout, fields.iter()).bytes();
ImmTy::from_uint(val, usize_layout())
}
UbChecks => ImmTy::from_bool(self.tcx.sess.ub_checks(), *self.tcx),
Copy link
Member Author

Choose a reason for hiding this comment

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

The type isn't even used here... this makes me wonder whether we should remove the type from the Rvalue::NullaryOp and instead have it in NullaryOp::SizeOf etc?

Cc @scottmcm

Copy link
Member

Choose a reason for hiding this comment

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

Sticking a type in NullaryOp probably complicates a bunch of places where they're lifetime-free and Copy, so I'm not enthusiastic about that.

TBH, if we're going to refactor stuff here, I'd love to just kill UnOp::SizeOf and UnOp::AlignOf entirely, and use associated consts on a magic trait, so it's just switchInt <T as Foo>::SIZE instead of needing pointless temporaries.

Since UbChecks is pretty weird -- it's a constant but it's not constant -- it might better to just split it out entirely since it works so differently from all the other UnOps...

Copy link
Member

Choose a reason for hiding this comment

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

I would like to see an attempt to reimplement UbChecks as a const... Somehow. It would make a lot more sense.

Copy link
Member Author

@RalfJung RalfJung Aug 6, 2024

Choose a reason for hiding this comment

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

Sticking a type in NullaryOp probably complicates a bunch of places where they're lifetime-free and Copy, so I'm not enthusiastic about that.

NullaryOp already has a 'tcx lifetime and Ty<'tcx> is Copy, so it doesn't change anything in this regard.

Copy link
Member Author

@RalfJung RalfJung Aug 6, 2024

Choose a reason for hiding this comment

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

Since UbChecks is pretty weird -- it's a constant but it's not constant -- it might better to just split it out entirely since it works so differently from all the other UnOps...

I would like to see an attempt to reimplement UbChecks as a const... Somehow. It would make a lot more sense.

It's not a constant. It can have different values in different compilation units, that's its entire point. So I think a NullaryOp makes a lot more sense for it than trying to make it a constant.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

r? compiler-errors @bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 5, 2024

📌 Commit 46896d6 has been approved by compiler-errors

It is now in the queue for this repository.

@rustbot rustbot assigned compiler-errors and unassigned TaKO8Ki Aug 5, 2024
@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 Aug 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#122049 (Promote riscv64gc-unknown-linux-musl to tier 2)
 - rust-lang#128580 (Use `ParamEnv::reveal_all` in CFI)
 - rust-lang#128688 (custom MIR: add support for tail calls)
 - rust-lang#128694 (Normalize when equating `dyn` tails in MIR borrowck)
 - rust-lang#128697 (interpret: move nullary-op evaluation into operator.rs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c53698b into rust-lang:master Aug 6, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 6, 2024
Rollup merge of rust-lang#128697 - RalfJung:nullary-op, r=compiler-errors

interpret: move nullary-op evaluation into operator.rs

We call it an operator, so we might as well treat it like one. :)

Also use more consistent naming for the "evaluate intrinsic" functions. "emulate" is really the wrong term, this *is* a genuine implementation of the intrinsic semantics after all.
@rustbot rustbot added this to the 1.82.0 milestone Aug 6, 2024
@RalfJung RalfJung deleted the nullary-op branch August 6, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants