12

Make `const_eval_select` a real intrinsic by fee1-dead · Pull Request #100759 ·...

 2 years ago
source link: https://github.com/rust-lang/rust/pull/100759
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Conversation

Member

@fee1-dead fee1-dead commented 20 days ago •

edited

This fixes issues where track_caller functions do not have nice panic
messages anymore when there is a call to the function, and uses the
MIR system to replace the call instead of dispatching via lang items.

Fixes #100696.

fmease, mati865, carols10cents, and matklad reacted with heart emoji All reactions

rustbot

added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

labels

20 days ago

Collaborator

rust-highfive commented 20 days ago

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

Collaborator

rustbot commented 20 days ago

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

rust-highfive

added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

20 days ago

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

fee1-dead

force-pushed the const_eval_select_real_intrinsic

branch 2 times, most recently from 79eca84 to 7aa26a1 Compare

18 days ago

Member

RalfJung commented 18 days ago

Another possible implementation plan would be to exploit that we have separate MIR for CTFE and runtime, and implement the intrinsic via lowering to a regular function call some place in the MIR pipeline. That would make supporting track_caller trivial. It'd also share the code to do that between codegen and CTFE.

Member

Author

fee1-dead commented 18 days ago

Another possible implementation plan would be to exploit that we have separate MIR for CTFE and runtime

Yes, but AFAIK there is no separate query for runtime-only MIR, right? Implementing it that way would require adding such a query and rewiring runtime MIR usage to that.

Member

RalfJung commented 18 days ago •

edited

The optimized_mir query is only used for runtime MIR, I think. But maybe that changed again. The hierarchy of MIR queries is historically an underdocumented mess, not sure if it improved over the last year.

But I am pretty sure that there is a fork in the road somewhere for MIR (where it becomes CTFE/runtime only), and we should be able to use that here.

Member

RalfJung commented 18 days ago

Yeah looks like mir_drops_elaborated_and_const_checked is the last MIR that is shared, and then mir_for_ctfe clones that while optimized_mir steals it.

Member

RalfJung commented 18 days ago

It is kind of a crude hack to have 'implementation' passes in optimized_mir, but it might be the best solution for this particular intrinsic.

Collaborator

rustbot commented 17 days ago

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Contributor

@oli-obk oli-obk left a comment

yea, I like this variant of doing it on the MIR

All reactions

Contributor

oli-obk commented 17 days ago

@bors r+

Contributor

bors commented 17 days ago

pushpin Commit 0d03c62 has been approved by oli-obk

It is now in the queue for this repository.

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

17 days ago

Member

Author

fee1-dead commented 17 days ago

@bors r- There was a test that changed to a no-op when I was debugging. I need to fix that

bors

added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

17 days ago

@@ -1034,6 +1034,7 @@ impl f32 {

}

}

#[inline(always)]

Contributor

@bjorn3 bjorn3 7 days ago •

edited

I don't think this is the right fix. I think the monomorphization collector needs to handle these functions even without #[inline(always)].

All reactions

Member

Author

@fee1-dead fee1-dead 7 days ago

The collector runs on instance_mir which runs on the shim mir or optimized_mir, which should already have the const_eval_select calls rewired.

All reactions

Contributor

@bjorn3 bjorn3 7 days ago

Yeah, no clue why it fails to work properly.

All reactions

Member

Author

@fee1-dead fee1-dead 7 days ago •

edited

I've tested with the arm target compiler and this is indeed a fix, so I guess this could just be some ghost bug that people who add const_eval_select to library functions worry about in the future.

All reactions

I don't think that's a sufficiently satisfying answer; the bug is at minimum exposed by these changes. We should understand why (even if we don't necessarily fix it here) and track fixing it.

All reactions

Contributor

@tmiasko tmiasko 7 days ago

The undefined reference is from compiler-builtins, which shouldn't have any link time dependencies on the core. We have been using inline to avoid this issue so far, so that seems fine to me, although personally I would prefer approach proposed in rust-lang/compiler-builtins#472.

fee1-dead, Mark-Simulacrum, and mati865 reacted with thumbs up emoji All reactions

Contributor

@bjorn3 bjorn3 7 days ago

Right, forgot about that.

All reactions

So looks like we do know what the problem is. Is there an issue open in https://github.com/rust-lang/compiler-builtins for that? If not, could you create one? Then the inline attribute here should reference that issue.

mati865 reacted with thumbs up emoji All reactions

Member

Author

@fee1-dead fee1-dead 5 days ago

I created an issue and linked to it from this attribute.

All reactions

Contributor

bors commented 5 days ago

umbrella The latest upstream changes (presumably #101396) made this pull request unmergeable. Please resolve the merge conflicts.

Member

RalfJung commented 4 days ago

@bors delegate+

bors r=oli-obk,RalfJung once CI is green

Contributor

bors commented 4 days ago

v@fee1-dead can now approve this pull request

Member

Author

fee1-dead commented 4 days ago

@bors r=oli-obk,RalfJung

Contributor

bors commented 4 days ago

pushpin Commit 65b685e has been approved by oli-obk,RalfJung

It is now in the queue for this repository.

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

4 days ago

Contributor

bors commented 4 days ago

hourglass Testing commit 65b685e with merge 9358d09...

Contributor

bors commented 4 days ago

sunny Test successful - checks-actions
Approved by: oli-obk,RalfJung
Pushing 9358d09 to master...

Collaborator

rust-timer commented 3 days ago

Finished benchmarking commit (9358d09): comparison URL.

Overall result: xwhite_check_mark regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions x
(primary)		 0.8% [0.3%, 1.4%] 12
Regressions x
(secondary)		 2.4% [0.2%, 4.1%] 9
Improvements white_check_mark
(primary)		 -1.2% [-3.0%, -0.6%] 7
Improvements white_check_mark
(secondary)		 -1.4% [-1.6%, -1.0%] 10
All xwhite_check_mark (primary) 0.1% [-3.0%, 1.4%] 19

Max RSS (memory usage)

Results

Cycles

Results

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

fbstj reacted with eyes emoji All reactions

rustbot

added the perf-regression Performance regressions label

3 days ago

fee1-dead

deleted the const_eval_select_real_intrinsic branch

3 days ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels
merged-by-bors This PR was explicitly merged by bors perf-regression Performance regressions 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects

None yet

Milestone

1.65.0

Development

Successfully merging this pull request may close these issues.

13 participants

report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK