-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Separate immediate and in-memory ScalarPair representation #118991
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,7 +179,10 @@ impl<'ll, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'_, 'll, 'tcx> { | |
unsafe { | ||
llvm::LLVMSetAlignment(load, align); | ||
} | ||
self.to_immediate(load, self.layout_of(tp_ty)) | ||
if !result.layout.is_zst() { | ||
self.store(load, result.llval, result.align); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this change necessary? Is it because there might now be trailing padding where previously there wasn’t any? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The volatile_load implementation blindly loads the value using the in-memory type, so it can produce loads of array and (arbitrary) struct types. These are not really valid immediates as far as rustc is concerned (and non-canonical as far as LLVM is concerned). The way this ends up getting handled is that if the value has Scalar ABI we convert it to an immediate, while everything else is left alone. After this change, this no longer works for ScalarPair ABI, which would need an adjustment. What this change does it to never treat the value as an immediate in the first place, and just directly store it back in in-memory representation. The implementation of volatile_load is really questionable in general (we really shouldn't be generating array/struct loads), but it's not really clear how it should be implemented given that we made the major design mistake of allowing volatile loads of arbitrary types, which is not a well-defined operation. We just leave it up to LLVM to interpret this in some way... |
||
} | ||
return; | ||
} | ||
sym::volatile_store => { | ||
let dst = args[0].deref(self.cx()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,25 +2,25 @@ | |
|
||
#![crate_type = "lib"] | ||
|
||
// CHECK: define{{.*}}{ i8, i8 } @pair_bool_bool(i1 noundef zeroext %pair.0, i1 noundef zeroext %pair.1) | ||
// CHECK: define{{.*}}{ i1, i1 } @pair_bool_bool(i1 noundef zeroext %pair.0, i1 noundef zeroext %pair.1) | ||
#[no_mangle] | ||
pub fn pair_bool_bool(pair: (bool, bool)) -> (bool, bool) { | ||
pair | ||
} | ||
|
||
// CHECK: define{{.*}}{ i8, i32 } @pair_bool_i32(i1 noundef zeroext %pair.0, i32 noundef %pair.1) | ||
// CHECK: define{{.*}}{ i1, i32 } @pair_bool_i32(i1 noundef zeroext %pair.0, i32 noundef %pair.1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, this is amazing! 🎉 Should save |
||
#[no_mangle] | ||
pub fn pair_bool_i32(pair: (bool, i32)) -> (bool, i32) { | ||
pair | ||
} | ||
|
||
// CHECK: define{{.*}}{ i32, i8 } @pair_i32_bool(i32 noundef %pair.0, i1 noundef zeroext %pair.1) | ||
// CHECK: define{{.*}}{ i32, i1 } @pair_i32_bool(i32 noundef %pair.0, i1 noundef zeroext %pair.1) | ||
#[no_mangle] | ||
pub fn pair_i32_bool(pair: (i32, bool)) -> (i32, bool) { | ||
pair | ||
} | ||
|
||
// CHECK: define{{.*}}{ i8, i8 } @pair_and_or(i1 noundef zeroext %_1.0, i1 noundef zeroext %_1.1) | ||
// CHECK: define{{.*}}{ i1, i1 } @pair_and_or(i1 noundef zeroext %_1.0, i1 noundef zeroext %_1.1) | ||
#[no_mangle] | ||
pub fn pair_and_or((a, b): (bool, bool)) -> (bool, bool) { | ||
// Make sure it can operate directly on the unpacked args | ||
|
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.
Hm, this
if
seems like a code smell to me. Since there are just two calls to this closure (as far as I can tell) and thisif
is the sole use of thei
argument, perhaps consider constructing thellptr
in the caller and passing it into the closure instead?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.
The
i
argument is also used to determinellty
.I've tried moving this into the caller, but this ends up being rather ugly, especially as
self
is captured by the closure, so we can't use it outside without further changes.