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

[Marco Elver via Phabricator via llvm-commits]

melver added a comment.

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

> I don't like using metadata like this.  Dropping metadata should generally preserve the semantics of the code.

Anything better for this without introducing new instructions? Would an argument be reasonable?

>> without resorting to inline assembly (which often results in poor codegen).
>
> Could you give an example of the resulting assembly with mustcontrol vs. this patch?

For one of the pathological cases:

  int x, y, z;                                                                                                                                                                                                                                                 
                                                                                                                                                                                                                                                                 
  int main(int argc, char *argv[]) {                                                                                                                                                                                                                           
      z = 42;                                                                                                                                                                                                                                                    
                                                                                                                                                                                                                                                                 
    volatile_if (READ_ONCE(x)) {                                                                                                                                                                                                                               
        WRITE_ONCE(y, z);                                                                                                                                                                                                                                        
      } else {                                                                                                                                                                                                                                                   
        WRITE_ONCE(y, z);                                                                                                                                                                                                                                        
      }                                                                                                                                                                                                                                                          
                                                                                                                                                                                                                                                                 
      return 0;                                                                                                                                                                                                                                                  
  }

Doing nothing:

  define dso_local i32 @main(i32 %argc, i8** nocapture readnone %argv) local_unnamed_addr #0 {
  entry:
    store i32 42, i32* @z, align 4, !tbaa !3
    %0 = load volatile i32, i32* @x, align 4, !tbaa !3
    store volatile i32 42, i32* @y, align 4, !tbaa !3
    ret i32 0
  }

No branch here.

Their latest proposal using compiler barriers (and not asmgoto):

  define dso_local i32 @main(i32 %0, i8** nocapture readnone %1) local_unnamed_addr #0 {
    store i32 42, i32* @z, align 4, !tbaa !2
    %3 = load volatile i32, i32* @x, align 4, !tbaa !2
    %4 = icmp eq i32 %3, 0
    br i1 %4, label %7, label %5
  
  5:                                                ; preds = %2
    tail call void asm sideeffect "", "i,~{memory},~{dirflag},~{fpsr},~{flags}"(i32 0) #1, !srcloc !6                            
    %6 = load i32, i32* @z, align 4, !tbaa !2
    br label %7
  
  7:                                                ; preds = %2, %5
    %8 = phi i32 [ %6, %5 ], [ 42, %2 ]
    store volatile i32 %8, i32* @y, align 4, !tbaa !2
    ret i32 0
  }

You can see the unnecessary load to z.

With mustcontrol:

  define dso_local i32 @main(i32 %argc, i8** nocapture readnone %argv) local_unnamed_addr #0 {
  entry:
    store i32 42, i32* @z, align 4, !tbaa !3
    %0 = load volatile i32, i32* @x, align 4, !tbaa !3
    %tobool.not = icmp eq i32 %0, 0
    br i1 %tobool.not, label %if.end, label %if.then
  
  if.then:                                          ; preds = %entry
    tail call void (...) @llvm.sideeffect(i64 42)
    br label %if.end
  
  if.end:                                           ; preds = %entry, %if.then
    store volatile i32 42, i32* @y, align 4, !tbaa !3
    ret i32 0
  }

Of course, the more common case is just

  if (READ_ONCE(..)) { WRITE_ONCE(...); }

but as soon as inline asm is involved, the full compiler barrier would also cause any data after the branch to be reloaded.

The bigger worry is that the full compiler barrier does not guarantee emission of a branch, and the asmgoto variant is pretty fragile. arm64 maintainers worry about LTO, and better compiler optimizations. It really begs for compiler support for the architectures where it is relevant. The main one being arm64, which ld->st can be ordered by a control dependency.

While the issue at hand is related to memory models, I've tried to steer clear of the C/C++ memory models because the Linux kernel defines its own memory model. Therefore, defining the new primitive at a lower-level that relates to generated code (closer to 'volatile' or e.g. 'musttail' which inspired the name) is a goal here. This will satisfy the Linux kernel's requirements and can use 'mustcontrol' as a building block for the Linux-kernel memory model (LKMM) [ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0124r6.html ].

I'm intending to define it as follows:

  | 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.
  |
  | This typically affects optimizations that would cause removal of a conditional
  | branch. However, it also ensures that a conditional branch and subsequent
  | instructions are not replaced with non-branching conditional instructions.
  |
  | Requesting the generation of a branch may be required in execution environments
  | where execution of a specific conditional branch inhibits speculation or has
  | observable side-effects of interest otherwise.

Please bear with me, I'm updating examples and documentation. I didn't think anybody would look at this while [WIP]. :-)
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103958