[Openmp-commits] [PATCH] D106310: [Libomptarget] Remove volatile from NVPTX work function

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

I think your reasoning is equivalent to:

  void barrier();
  
  static int g;
  
  void set(int x) { g = x; }
  
  int get()
  {   
      int t = g;
      barrier();
      return g + t;
  }

If the barrier() call is commented out, the loads in get() fold. If the variable is volatile, both loads are emitted.

If the external barrier() call is present, both loads are emitted.

If the barrier not external, and does not change that global, e.g.

  int other;
  extern "C" void barrier(){other++;}

then the generated IR only reads the variable once again.

  define dso_local i32 @get() local_unnamed_addr #0 {
    %1 = load i32, i32* @_ZL1g, align 4, !tbaa !3
    %2 = load i32, i32* @other, align 4, !tbaa !3
    %3 = add nsw i32 %2, 1
    store i32 %3, i32* @other, align 4, !tbaa !3
    %4 = shl nsw i32 %1, 1
    ret i32 %4
  }

So I think, at present, the 'barrier' call works as a thread fence as far as the compiler is concerned, in that it doesn't move loads across it.

Given better cross function analysis, would the property of 'any call => fence' hold? I'm not confident it holds today within LLVM though I think it must do in clang.

In particular, as far as I can tell C and C++ require synchronisation via atomic variables, not merely fences on non-atomics, and LLVM tends to reflect C++ semantics. So I think one thread writing the variable while others read is a data race and we are avoiding being burned by that by conservative optimisations around function calls.

Changing the loads/stores on this variable to relaxed atomic firmly kills the UB data race, and will interact properly with llvm.fence (and hopefully) the nvptx barrier.

So I think we should use relaxed load/stores where we currently use volatile variables, instead of relying on clang not miscompiling our data race.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106310