[PATCH] D103958: [WIP] Support MustControl conditional control attribute

Thu Jun 10 05:32:11 PDT 2021

melver added a comment.

In D103958#2809353 <https://reviews.llvm.org/D103958#2809353>, @efriedma wrote:

> You could break `__builtin_load_with_control_dependency(x)` into something like `__builtin_control_dependency(READ_ONCE(x))`.  I don't think any transforms will touch that in practice, even if it isn't theoretically sound.  The rest of my suggestion still applies to that form, I think.  They key point is that the compiler just needs to ensure some branch consumes the loaded value; it doesn't matter which branch it is.

Because the original inline asm version was pretty similar (they just called it `volatile_cond()`), I think `__builtin_control_dependency()` is equivalent. Actually a later suggestion just called it `__builtin_ctrl_depends()`: https://lkml.kernel.org/r/YL9TEqealhxBBhoS@hirez.programming.kicks-ass.net

It was nacked by GCC devs, who expressed concern that it seems impossible to guarantee a branch is emitted but also way too difficult to specify. The emitting-branch part seems straightforward, as you suggested.

I think implementation-wise, we can probably use your idea either way. I just worry about defined semantics, see below.

> The theoretical problem with separating the load from the branch is that it imposes an implicit contract: the branch has to use the value of the load as input, not an equivalent value produced some other way.  This is the general problem with C++11 consume ordering, which nobody has tried to tackle.

Indeed. Which is why I wanted to steer clear of a __builtin that talks about control-dependencies directly. There are 2 challenges:

1. Defining the value used to establish a control dependency, e.g. the load later writes depend on (kernel only defines writes to be ctrl-dependently ordered).
2. Defining later ops that are control-dependent. With an expression like the __builtin, this could be any operation after, or it becomes too hard to define:

  x = __builtin_control_dependency(expr); // __builtin_control_dependency establishes an ordering edge between loads in expr and later ops
  y = 1; // control dependently ordered, although there is no explicit control statement yet...
  if (x) { z = 1; } // ... this code is only interested in z=1 to be control-dependently ordered.

Both are hard to define, as you suggested it's similar to consume which was also my worry.

Therefore, to get something simple that works and isn't doomed to a definition that is unimplementable, I tried to just talk about the control statement and the fact a branch must be emitted. In theory, (1) might still be a problem, but in practice the compiler has no other way than to use the loaded value if that value was loaded through an __atomic or volatile or similar.

Limiting ourselves to an attribute on control statements solves (2), because we can say that "the conditional branch is placed *before* conditionally executed instructions". Either that, or we make the __builtin give an error if used outside the condition of a control statement.

----

Given we've already gotten this far, I will summarize the options:

A. `__builtin_load_with_control_dependency()` -- this appears to solve the problem (1) above, but not (2). This approach seems unappealing if we want to solve this for the Linux kernel, because the whole point of compiler support was to avoid more read-primitives (with new primitives they say they'd just upgrade these to acquire and be done with it).

B. `__builtin_control_dependency()` -- would be nice if this would work, but I think it suffers from problems (1), and (2) above and is very hard to define properly.
B1 <https://reviews.llvm.org/B1>. Do this but constrain it to only be usable as conditions in control statements, which would solve (2) at least.

C. `[[mustcontrol]]`:

  Marking a conditional control statement as ``mustcontrol`` indicates that the
  compiler must generate a conditional branch in machine code, and such
  conditional branch is placed *before* conditionally executed instructions. The
  attribute may be ignored if the condition of the control statement is a
  constant expression.

D. But we can also just rename it to `[[control_dependency]]` if that's clearer. It looks like it's the same as B1 <https://reviews.llvm.org/B1> minus the artificial constraint; implementation should be similar. It'd allow for the same arch-dependent omission if an arch does not care about control dependencies. But I feel that, at least for the Linux kernel, they prefer having as much control over codegen as possible, regardless of arch. Because if there's an arch-agnostic way of doing this, we get it for free for POWER and Armv7.

All we'd like is a robust primitive, without overcomplicating things.

What do you recommend?

Thanks.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103958/new/

https://reviews.llvm.org/D103958