hint: Make spin_loop_hint a side-effect for all targets

[josephlr]

In rust-lang/rust-clippy#6161 and rust-lang/rust-clippy#6162 we were discussing changes to the empty_loop lint. That lint prohibits code like:

loop { }

for two reasons:

• Hot dead-looping unnecessarily wastes CPU cycles
• An LLVM codegen bug means that this code can cause UB

For std targets we recommend either panicking or replacing the code with:

loop {
  std::thread::sleep(...); // Could also be std::thread::yield_now();
}

For no_std targets, in an ideal world, we would just recommend panicking or replacing the code with:

loop {
  core::sync::atomic::spin_loop_hint();
}

While this would fix the performance issue (on some platforms), it wouldn't fix the LLVM codegen issue on all platforms. This change makes it so spin_loop_hint is always a side-effect from LLVM's perspective. This makes giving recommendations to users much easier. This change has no runtime costs (except that is disables optimizations we want disabled anyway). It also makes the behavior more consistent across targets (as spin_loop_hint is already a side-effect on x86 and (most) ARM targets).

Without this change, we would have to recommend very weird, unsafe things (like read_volatile). Furthermore, those recommendations would become stale once the LLVM codegen bug was fixed. With this change, we can recommend a fix that still makes sense once LLVM is fixed.

CC: @jonas-schievink @RalfJung @jyn514 @flip1995

Signed-off-by: Joe Richey [email protected]

[rust-highfive]

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

I think I would rather we change codegen to explicitly emit an LLVM side effect on loop {}; that should be fairly easy to do, just no one has had the bandwidth to do so yet. I believe we had broad agreement that a partial fix would be good here even if we don't have a full fix available yet.

I don't think we should pessimize spin loop hint with inline asm volatile; that seems like it would potentially discourage it's use. Maybe it's not really important from llvm's perspective, though. Seems like we'd be hard pressed to tell since effects are likely invisible in most programs anyway.

[josephlr]

Maybe it's not really important from llvm's perspective, though.

I wasn't able to get any sort of different codegen with this change. I would suspect that it probably does the exact same thing as marking the loop with llvm.sideeffect. I doubt that this would present an issue in practice (especially because it's already a side effect on the most common architectures).

I think I would rather we change codegen to explicitly emit an LLVM side effect on loop {}

This was my original idea, but after looking into it, it seemed hard to do in practice (but I don't have a lot of experience with the codegen part of rustc, so maybe it's not that hard).

[josephlr]

I think I would rather we change codegen to explicitly emit an LLVM side effect on loop {}

Another thing that complicates this is that we don't want:

loop {
  core::sync::atomic::spin_loop_hint();
}

to be a regression w.r.t. the LLVM bug compared with loop { }. On non x86/ARM platfroms, spin_loop_hint contains no code, so maybe this is fine.

[RalfJung]

Seems like a reasonable hack to me, though I cannot comment on how much this would penalize other users of spin_loop_hint. I would think you'd usually in situations involving synchronization, where you have barriers or corresponding atomic instructions anyway and the compiler is already precluded from reordering things. But maybe this implementation prevents too much reordering, I wouldn't know.

Without this change, we would have to recommend very weird, unsafe things (like read_volatile). Furthermore, those recommendations would become stale once the LLVM codegen bug was fixed. With this change, we can recommend a fix that still makes sense once LLVM is fixed.

How does this still make any sense once LLVM is fixed? If you don't actually want a spinloop but just a program running forever, loop {} is fully correct then.

[josephlr]

How does this still make any sense once LLVM is fixed? If you don't actually want a spinloop but just a program running forever, loop {} is fully correct then.

So the lint is actually two separate issues. First, a correctness issue: this LLVM bug. The second is a style/performance issue, where you usually don't want a "hot" deadloop, you want to do something in the loop to relax/sleep/pause the thread. For no_std, we can't sleep, but we can use this hint (which emits a reasonable instruction on x86 and ARM).

The linked clippy PR explains this in greater detail.

[josephlr]

After reading more about PAUSE, https://software.intel.com/content/www/us/en/develop/articles/benefitting-power-and-performance-sleep-loops.html, we really shouldn't be recommending this as a way to reduce power in a dead-loop. On x86, PAUSE mostly effects the cache system by lowering the hyperthread's priority for cache requests. This is how it reduces power. I was unable to find any documentation that suggests dead-looping with a pause has any affect on power consumption (provided you're not waiting on some memory value to change).

Closing this. As @RalfJung mentioned, it doesn't make any sense once the LLVM bug is fixed.