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.

Sign up for GitHub

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

Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary #122220

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary #122220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion compiler/rustc_codegen_llvm/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,9 +1132,15 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
&mut self,
op: rustc_codegen_ssa::common::AtomicRmwBinOp,
dst: &'ll Value,
src: &'ll Value,
mut src: &'ll Value,
order: rustc_codegen_ssa::common::AtomicOrdering,
) -> &'ll Value {
// The only RMW operation that LLVM supports on pointers is compare-exchange.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we report this somewhere upstream? Or at least track it with a FIXME or issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Which operations does Rust use on pointers? To the most part these just don't make sense, at least not in a straightforward way (even something like add would run up against the provenance question).

Copy link
Member Author

@saethlin saethlin Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that compare-exchange is kind of the odd-one-out here (in that it makes sense and the other RMW ops do not).

Up to you where the check for it should live 🤷 I decided to sink the branch because otherwise we have an odd interface where we accept an enum but you better only pass one variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

The check is fine -- my comment was to answer @oli-obk's question on whether this needs to be reported upstream, and I think the answer is "no".

if self.val_ty(src) == self.type_ptr()
&& op != rustc_codegen_ssa::common::AtomicRmwBinOp::AtomicXchg
{
src = self.ptrtoint(src, self.type_isize());
Copy link
Member

Choose a reason for hiding this comment

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

If there's a ptrtoint for the argument, doesn't there also have to be an inttoptr for the result?

Copy link
Member Author

@saethlin saethlin Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hunh. It sure seems like an inttoptr should be required, but the previous implementation didn't have one either, right? I think I'm going to add the conversion back to pointer type anyway with a codegen test.

}
unsafe {
llvm::LLVMBuildAtomicRMW(
self.llbuilder,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub enum RealPredicate {
RealPredicateTrue,
}

#[derive(Copy, Clone)]
#[derive(Copy, Clone, PartialEq)]
pub enum AtomicRmwBinOp {
AtomicXchg,
AtomicAdd,
Expand Down
50 changes: 10 additions & 40 deletions compiler/rustc_codegen_ssa/src/mir/intrinsic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,14 +350,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if int_type_width_signed(ty, bx.tcx()).is_some() || ty.is_unsafe_ptr() {
let weak = instruction == "cxchgweak";
let dst = args[0].immediate();
let mut cmp = args[1].immediate();
let mut src = args[2].immediate();
if ty.is_unsafe_ptr() {
// Some platforms do not support atomic operations on pointers,
// so we cast to integer first.
cmp = bx.ptrtoint(cmp, bx.type_isize());
src = bx.ptrtoint(src, bx.type_isize());
}
let cmp = args[1].immediate();
let src = args[2].immediate();
let (val, success) = bx.atomic_cmpxchg(
dst,
cmp,
Expand Down Expand Up @@ -385,26 +379,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let layout = bx.layout_of(ty);
let size = layout.size;
let source = args[0].immediate();
if ty.is_unsafe_ptr() {
// Some platforms do not support atomic operations on pointers,
// so we cast to integer first...
let llty = bx.type_isize();
let result = bx.atomic_load(
llty,
source,
parse_ordering(bx, ordering),
size,
);
// ... and then cast the result back to a pointer
bx.inttoptr(result, bx.backend_type(layout))
} else {
bx.atomic_load(
bx.backend_type(layout),
source,
parse_ordering(bx, ordering),
size,
)
}
bx.atomic_load(
bx.backend_type(layout),
source,
parse_ordering(bx, ordering),
size,
)
} else {
invalid_monomorphization(ty);
return Ok(());
Expand All @@ -415,13 +395,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let ty = fn_args.type_at(0);
if int_type_width_signed(ty, bx.tcx()).is_some() || ty.is_unsafe_ptr() {
let size = bx.layout_of(ty).size;
let mut val = args[1].immediate();
let val = args[1].immediate();
let ptr = args[0].immediate();
if ty.is_unsafe_ptr() {
// Some platforms do not support atomic operations on pointers,
// so we cast to integer first.
val = bx.ptrtoint(val, bx.type_isize());
}
bx.atomic_store(val, ptr, parse_ordering(bx, ordering), size);
} else {
invalid_monomorphization(ty);
Expand Down Expand Up @@ -465,12 +440,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let ty = fn_args.type_at(0);
if int_type_width_signed(ty, bx.tcx()).is_some() || ty.is_unsafe_ptr() {
let ptr = args[0].immediate();
let mut val = args[1].immediate();
if ty.is_unsafe_ptr() {
// Some platforms do not support atomic operations on pointers,
// so we cast to integer first.
val = bx.ptrtoint(val, bx.type_isize());
}
let val = args[1].immediate();
bx.atomic_rmw(atom_op, ptr, val, parse_ordering(bx, ordering))
} else {
invalid_monomorphization(ty);
Expand Down
Loading