Skip to content

WIP: Add in atomic_{min,max}_x intrinsics #1653

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

Closed
wants to merge 3 commits into from
Closed

WIP: Add in atomic_{min,max}_x intrinsics #1653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57ef47a
Add in `atomic_{min,max}_x` intrinsics
GregBowyer Dec 17, 2020
8493530
Add test for `atomic_{min,max}`
GregBowyer Dec 18, 2020
7f68b3d
Include `atomic_u{min,max}` in shims as well
GregBowyer Dec 18, 2020
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
29 changes: 29 additions & 0 deletions src/data_race.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,35 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
Ok(old)
}

/// Perform an conditional atomic exchange with a memory place and a new
/// scalar value, the old value is returned.
fn atomic_min_max_scalar(
&mut self,
place: MPlaceTy<'tcx, Tag>,
rhs: ImmTy<'tcx, Tag>,
min: bool,
atomic: AtomicRwOp,
) -> InterpResult<'tcx, ImmTy<'tcx, Tag>> {
let this = self.eval_context_mut();

let old = this.allow_data_races_mut(|this| this.read_immediate(place.into()))?;

// `binary_op` will bail if either of them is not a scalar.
let cond = if min {
this.overflowing_binary_op(mir::BinOp::Lt, old, rhs)?.0
} else {
this.overflowing_binary_op(mir::BinOp::Gt, old, rhs)?.0
};

if cond.to_bool()? {
this.allow_data_races_mut(|this| this.write_immediate(*rhs, place.into()))?;
}

this.validate_atomic_rmw(place, atomic)?;
// Return the old value.
Ok(old)
}

/// Perform an atomic compare and exchange at a given memory location.
/// On success an atomic RMW operation is performed and on failure
/// only an atomic read occurs.
Expand Down
45 changes: 45 additions & 0 deletions src/shims/intrinsics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,26 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
"atomic_xsub_acqrel" => this.atomic_op(args, dest, BinOp::Sub, false, AtomicRwOp::AcqRel)?,
"atomic_xsub_relaxed" => this.atomic_op(args, dest, BinOp::Sub, false, AtomicRwOp::Relaxed)?,

"atomic_min" => this.atomic_min_max(args, dest, true, AtomicRwOp::SeqCst)?,
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything fundamentally different between atomic_op and atomic_min_max? Would it make sense to merge them into a single function? As we just established, they should be basically the same in terms of synchronization behavior, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK The atomic_op appears to take its output from binary_op with overflow which I think means that for LT / GT would result in new becoming the condition of the branch (e.g. 1) rather than being max or min

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry if I am missing something I am obviously super new to the miri codebase

Copy link
Member

Choose a reason for hiding this comment

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

Right, so currently it uses mir::BinOp to determine the atomic_op. But we could instead use an enum like

enum AtomicOp {
  MirOp(mir::BinOp),
  Max,
  Min
}

and then we should be able to share much more code -- maybe?

Sorry if I am missing something I am obviously super new to the miri codebase

Don't worry, these are good questions. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am ok with that, I was under the impression the style was to keep towards mir more directly. I will play with that a bit when I figure out the test case and my test failures.

I guess the other option is to match on mir::BinOp::{Lt, Gt} too which removes the need for a wrapper and punts the specifics to atomic_op I will also play with that

Copy link
Member

Choose a reason for hiding this comment

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

Lt is for less-than though, and this is not "atomic less-than", it is "atomic min". Using Lt for this seems rather hacky.

I was under the impression the style was to keep towards mir more directly.

That's just what worked well so far.^^ But given that all the fetch_ operations behave very similarly, I feel we really should share as much code between them as we can.

"atomic_min_acq" => this.atomic_min_max(args, dest, true, AtomicRwOp::Acquire)?,
"atomic_min_rel" => this.atomic_min_max(args, dest, true, AtomicRwOp::Release)?,
"atomic_min_acqrel" => this.atomic_min_max(args, dest, true, AtomicRwOp::AcqRel)?,
"atomic_min_relaxed" => this.atomic_min_max(args, dest, true, AtomicRwOp::Relaxed)?,
"atomic_max" => this.atomic_min_max(args, dest, false, AtomicRwOp::SeqCst)?,
"atomic_max_acq" => this.atomic_min_max(args, dest, false, AtomicRwOp::Acquire)?,
"atomic_max_rel" => this.atomic_min_max(args, dest, false, AtomicRwOp::Release)?,
"atomic_max_acqrel" => this.atomic_min_max(args, dest, false, AtomicRwOp::AcqRel)?,
"atomic_max_relaxed" => this.atomic_min_max(args, dest, false, AtomicRwOp::Relaxed)?,
"atomic_umin" => this.atomic_min_max(args, dest, true, AtomicRwOp::SeqCst)?,
Copy link
Member

Choose a reason for hiding this comment

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

What are these about? If "u" is for "unsigned", then the code needs to be different... < behaves differently in the same bit patterns depending on whether the values are signed or unsigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

;) I was just getting to that, you are fast on the review

Copy link
Member

Choose a reason for hiding this comment

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

:D

Please make sure to add a test that does the max of -1 and 1 (signed) and the min of 1 and usize::MAX (unsigned), to make sure the code uses the signs correctly.

"atomic_umin_acq" => this.atomic_min_max(args, dest, true, AtomicRwOp::Acquire)?,
"atomic_umin_rel" => this.atomic_min_max(args, dest, true, AtomicRwOp::Release)?,
"atomic_umin_acqrel" => this.atomic_min_max(args, dest, true, AtomicRwOp::AcqRel)?,
"atomic_umin_relaxed" => this.atomic_min_max(args, dest, true, AtomicRwOp::Relaxed)?,
"atomic_umax" => this.atomic_min_max(args, dest, false, AtomicRwOp::SeqCst)?,
"atomic_umax_acq" => this.atomic_min_max(args, dest, false, AtomicRwOp::Acquire)?,
"atomic_umax_rel" => this.atomic_min_max(args, dest, false, AtomicRwOp::Release)?,
"atomic_umax_acqrel" => this.atomic_min_max(args, dest, false, AtomicRwOp::AcqRel)?,
"atomic_umax_relaxed" => this.atomic_min_max(args, dest, false, AtomicRwOp::Relaxed)?,

// Query type information
"assert_zero_valid" |
Expand Down Expand Up @@ -594,6 +614,31 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
self.atomic_compare_exchange(args, dest, success, fail)
}

fn atomic_min_max(
&mut self, args: &[OpTy<'tcx, Tag>], dest: PlaceTy<'tcx, Tag>,
min: bool,
atomic: AtomicRwOp
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();

let &[place, rhs] = check_arg_count(args)?;
let place = this.deref_operand(place)?;
if !place.layout.ty.is_integral() {
bug!("Atomic arithmetic operations only work on integer types");
}
let rhs = this.read_immediate(rhs)?;

// Check alignment requirements. Atomics must always be aligned to their size,
// even if the type they wrap would be less aligned (e.g. AtomicU64 on 32bit must
// be 8-aligned).
let align = Align::from_bytes(place.layout.size.bytes()).unwrap();
this.memory.check_ptr_access(place.ptr, place.layout.size, align)?;

let old = this.atomic_min_max_scalar(place, rhs, min, atomic)?;
this.write_immediate(*old, dest)?; // old value is returned
Ok(())
}

fn float_to_int_unchecked<F>(
&self,
f: F,
Expand Down
14 changes: 14 additions & 0 deletions tests/run-pass/atomic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ fn atomic_u64() {
Ok(1)
);
assert_eq!(ATOMIC.load(Relaxed), 0x100);

assert_eq!(ATOMIC.fetch_max(0x10, SeqCst), 0x100);
assert_eq!(ATOMIC.fetch_max(0x100, SeqCst), 0x100);
assert_eq!(ATOMIC.fetch_max(0x1000, SeqCst), 0x100);
assert_eq!(ATOMIC.fetch_max(0x1000, SeqCst), 0x1000);
assert_eq!(ATOMIC.fetch_max(0x2000, SeqCst), 0x1000);
assert_eq!(ATOMIC.fetch_max(0x2000, SeqCst), 0x2000);

assert_eq!(ATOMIC.fetch_min(0x2000, SeqCst), 0x2000);
assert_eq!(ATOMIC.fetch_min(0x2000, SeqCst), 0x2000);
assert_eq!(ATOMIC.fetch_max(0x1000, SeqCst), 0x2000);
assert_eq!(ATOMIC.fetch_max(0x1000, SeqCst), 0x1000);
assert_eq!(ATOMIC.fetch_max(0x100, SeqCst), 0x1000);
assert_eq!(ATOMIC.fetch_max(0x10, SeqCst), 0x100);
}

fn atomic_fences() {
Expand Down