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

Add a distinct OperandValue::ZeroSized variant for ZSTs #111318

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented May 7, 2023

These tend to have special handling in a bunch of places anyway, so the variant helps remember that. And I think it's easier to grok than Aggregates sometimes being Immediates (after all, I previously got that wrong and caused #109992). As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in OperandValue::Immediates.

Inspired by #110021 (comment), so
r? @compiler-errors

@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 May 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2023

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@@ -220,31 +223,33 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let fake_place = PlaceRef::new_sized_aligned(cast_ptr, cast, align);
Some(bx.load_operand(fake_place).val)
}
OperandValue::ZeroSized => {
let OperandValueKind::ZeroSized = operand_kind else {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be an assert instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could. I wrote it this way for structural similarity with the other arms.

Maybe I should have made this be match (operand.val, operand_kind) instead? Unsure.

Copy link
Member

Choose a reason for hiding this comment

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

Itś not that big of a deal, both branches are pretty short

@@ -220,31 +223,33 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let fake_place = PlaceRef::new_sized_aligned(cast_ptr, cast, align);
Some(bx.load_operand(fake_place).val)
}
OperandValue::ZeroSized => {
let OperandValueKind::ZeroSized = operand_kind else {
Copy link
Member

Choose a reason for hiding this comment

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

Itś not that big of a deal, both branches are pretty short

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 29, 2023

📌 Commit f315f57db8721e0993d9866c2cd5c3cae0fb6700 has been approved by compiler-errors

It is now in the queue for this repository.

@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 May 29, 2023
@bors
Copy link
Contributor

bors commented May 29, 2023

⌛ Testing commit f315f57db8721e0993d9866c2cd5c3cae0fb6700 with merge 28d692dccbadccb687ae571daf3591510a451529...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 29, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2023
@scottmcm
Copy link
Member Author

Looks like a legit failure; I'll dig in.

@bors r-

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2023
@scottmcm scottmcm force-pushed the operand-value-poison branch 2 times, most recently from b95bfa5 to 9495095 Compare May 29, 2023 08:12
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels May 29, 2023
@scottmcm
Copy link
Member Author

@bors
Copy link
Contributor

bors commented May 29, 2023

📌 Commit 9495095 has been approved by compiler-errors

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 29, 2023
@scottmcm scottmcm removed A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels May 29, 2023
Noratrieb added a commit to Noratrieb/rust that referenced this pull request May 30, 2023
…ompiler-errors

Add a distinct `OperandValue::ZeroSized` variant for ZSTs

These tend to have special handling in a bunch of places anyway, so the variant helps remember that.  And I think it's easier to grok than `Aggregate`s sometimes being `Immediates` (after all, I previously got that wrong and caused rust-lang#109992).  As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in `OperandValue::Immediate`s.

Inspired by rust-lang#110021 (comment), so
r? `@compiler-errors`
Noratrieb added a commit to Noratrieb/rust that referenced this pull request May 30, 2023
…ompiler-errors

Add a distinct `OperandValue::ZeroSized` variant for ZSTs

These tend to have special handling in a bunch of places anyway, so the variant helps remember that.  And I think it's easier to grok than `Aggregate`s sometimes being `Immediates` (after all, I previously got that wrong and caused rust-lang#109992).  As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in `OperandValue::Immediate`s.

Inspired by rust-lang#110021 (comment), so
r? ``@compiler-errors``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 31, 2023
…ompiler-errors

Add a distinct `OperandValue::ZeroSized` variant for ZSTs

These tend to have special handling in a bunch of places anyway, so the variant helps remember that.  And I think it's easier to grok than `Aggregate`s sometimes being `Immediates` (after all, I previously got that wrong and caused rust-lang#109992).  As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in `OperandValue::Immediate`s.

Inspired by rust-lang#110021 (comment), so
r? ```@compiler-errors```
@matthiaskrgr
Copy link
Member

@bors r- #112130

@bors 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 May 31, 2023
@scottmcm scottmcm closed this May 31, 2023
@scottmcm scottmcm reopened this May 31, 2023
@rust-log-analyzer

This comment has been minimized.

These tend to have special handling in a bunch of places anyway, so the variant helps remember that.  And I think it's easier to grok than non-Scalar Aggregates sometimes being `Immediates` (like I got wrong and caused 109992).  As a minor bonus, it means we don't need to generate poison LLVM values for them to pass around in `OperandValue::Immediate`s.
@scottmcm
Copy link
Member Author

scottmcm commented Jun 1, 2023

Rebased to resolve merge with #111768
@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jun 1, 2023

📌 Commit bf36193 has been approved by compiler-errors

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
 - rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
 - rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
 - rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
 - rust-lang#112024 (Don't suggest break through nested items)
 - rust-lang#112128 (Don't compute inlining status of mono items in advance.)
 - rust-lang#112141 (remove reference to Into in ? operator core/std docs, fix rust-lang#111655)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 03d4299 into rust-lang:master Jun 1, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 1, 2023
@scottmcm scottmcm deleted the operand-value-poison branch June 2, 2023 03:12
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