Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
See through aggregates in GVN #116270
See through aggregates in GVN #116270
Changes from 1 commit
d284059
afd631c
38c86b0
db9bd9b
9389373
692e528
48d2157
f110f22
23d4857
80a5e85
dbf9ea3
8162dc2
ebc87bf
ff6812c
fbf0a0c
59235a7
e3538d1
f08dc9b
5e78b9c
f6aa3ee
50559ce
ac0228d
c4cc9ca
d80eb3a
eda1928
72f0e0e
8561618
24be433
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path should be unreachable now? Why is there a fallback path here? I can't see why we'd ever want to copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
op
could be an immediate scalar pair. That wouldn't be caught by the scalar path, nor the mplace path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems nicer to have a 3-way match than a series of fallbacks, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For scalars, we may want to encode them in MIR as a
ConstValue::Scalar
, even if the interpreter backs it by aMPlace
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure. What I dislike is to have this look like "try A; try B; try C", I'd much rather have a very clear case distinction showing when we use which case. In particular since the code currently completely mixes up checks that are actually needed and errors that can never happen and certainly shouldn't lead to a fallback.
So you can do something like
if let Abi::Scalar(abi::Scalar::Initialized { .. }) = op.layout.abi
. In that case always use ConstValue::Scalar, never fall back to anything else, with a comment explaining why we are doing this.op.as_mplace_or_imm()
, and if you have an mplace use the fast path, otherwise the copy, with a comment explaining this can happen for ScalarPair and ideally we'd have a more efficient representation for them but currently we don't (Cc Further possibilities for mir::Constant cleanup #115877)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems easier to check the allocation, given that you have an
alloc_id
?Also please add comments explaining that this is just a temporary hack to work around #79738.