[mlir] do not use llvm.cmpxchg with floats

[ftynse]

Summary

According to the LLVM Language Reference, 'cmpxchg' accepts integer or pointer
types. Several MLIR tests were using it with floats as it appears possible to
programmatically construct and print such an instruction, but it cannot be
parsed back. Use integers instead.

Depends On D85899

[jfb]

I'd expect us to go the other way, and allow this. There have been patches in the past to allow usage of atomics with float. Could you look with their authors and see what they advise here?

[flaub]

Additionally, there are some operations that can't feasibly be implemented without floating-point cmpxchg working. I can see wanting to unblock these tests, but without changes to the verification of the GenericAtomicRMWOp, it will be possible for users to create floating-point cmpxchg.

[ftynse]

In D85900#2216323, @jfb wrote:

I'd expect us to go the other way, and allow this. There have been patches in the past to allow usage of atomics with float. Could you look with their authors and see what they advise here?

LLVM is inconsistent about this: LangRef says integers and pointers are allowed, and one cannot parse cmpxchg with floats, but one apparently _can_ build such instruction. I brought it up here http://lists.llvm.org/pipermail/llvm-dev/2020-August/144275.html, let's continue there. I prefer MLIR to be conservative.

In D85900#2216458, @flaub wrote:

Additionally, there are some operations that can't feasibly be implemented without floating-point cmpxchg working. I can see wanting to unblock these tests, but without changes to the verification of the GenericAtomicRMWOp, it will be possible for users to create floating-point cmpxchg.

We need some digging at how much is this actually supported in LLVM. I haven't had time so far, help appreciated.

If GenericAtomicRMWOp produces is rewritten to ops that fail the verifier, the conversion will fail, we will not have IR with invalid ops in it.

[jfb]

See for example r360421 D58251 D50491 D26266 D26266 r264845 D15471
Maybe talk to folks such as @reames ?

[ftynse]

In D85900#2216949, @jfb wrote:

See for example r360421 D58251 D50491 D26266 D26266 r264845 D15471
Maybe talk to folks such as @reames ?

Can we have this discussion on llvm-dev, http://lists.llvm.org/pipermail/llvm-dev/2020-August/144275.html, rather than in a MLIR review thread that I assume most relevant folks won't follow?

I feel quite strongly against allowing MLIR LLVM dialect to directly contradict LLVM IR spec. https://llvm.org/docs/LangRef.html#id220 says "The type of ‘<cmp>’ must be an integer or pointer type whose bit width is a power of two greater than or equal to eight and less than or equal to a target-specific size limit", which should be reflected as AnyTypeOf<[LLVM_AnyPow2Integer, LLVM_AnyPointer]>; in MLIR LLVM op definition. If LLVM IR spec changes, I am happy to reflect that in MLIR op definition, it's a one-liner change. Note that it does not preclude other dialects, including those that model LLVM IR intrinsics, from having any operations they want, they just shouldn't pretend they map LLVM IR into an MLIR dialect.

[flaub]

Looks like xchg can handle floats but not cmpxchg, so I'm in favor of proceeding with this. Might want to also adjust the IR to reflect the tighter constraints.

See: https://lists.llvm.org/pipermail/llvm-dev/2020-August/144300.html

mlir/test/Conversion/StandardToLLVM/convert-to-llvmir.mlir

func @atomic_rmw(%I : memref<10xi32>, %ival : i32, %F : memref<10xf32>, %fval : f32, %i : index) {
   atomic_rmw "addf" %fval, %F[%i] : (f32, memref<10xf32>) -> f32
   // CHECK: llvm.atomicrmw fadd %{{.*}}, %{{.*}} acq_rel
   return
 }
 
 // -----
 
 // CHECK-LABEL: func @generic_atomic_rmw
-func @generic_atomic_rmw(%I : memref<10xf32>, %i : index) -> f32 {
-  %x = generic_atomic_rmw %I[%i] : memref<10xf32> {
-    ^bb0(%old_value : f32):
-      %c1 = constant 1.0 : f32
-      atomic_yield %c1 : f32
-  }
-  // CHECK: [[init:%.*]] = llvm.load %{{.*}} : !llvm.ptr<float>
-  // CHECK-NEXT: llvm.br ^bb1([[init]] : !llvm.float)
-  // CHECK-NEXT: ^bb1([[loaded:%.*]]: !llvm.float):
-  // CHECK-NEXT: [[c1:%.*]] = llvm.mlir.constant(1.000000e+00 : f32)
+func @generic_atomic_rmw(%I : memref<10xi32>, %i : index) -> i32 {
+  %x = generic_atomic_rmw %I[%i] : memref<10xi32> {
+    ^bb0(%old_value : i32):
+      %c1 = constant 1 : i32
+      atomic_yield %c1 : i32
+  }
+  // CHECK: [[init:%.*]] = llvm.load %{{.*}} : !llvm.ptr<i32>
+  // CHECK-NEXT: llvm.br ^bb1([[init]] : !llvm.i32)
+  // CHECK-NEXT: ^bb1([[loaded:%.*]]: !llvm.i32):
+  // CHECK-NEXT: [[c1:%.*]] = llvm.mlir.constant(1 : i32)
   // CHECK-NEXT: [[pair:%.*]] = llvm.cmpxchg %{{.*}}, [[loaded]], [[c1]]
-  // CHECK-SAME:                    acq_rel monotonic : !llvm.float
+  // CHECK-SAME:                    acq_rel monotonic : !llvm.i32
   // CHECK-NEXT: [[new:%.*]] = llvm.extractvalue [[pair]][0]
   // CHECK-NEXT: [[ok:%.*]] = llvm.extractvalue [[pair]][1]
-  // CHECK-NEXT: llvm.cond_br [[ok]], ^bb2, ^bb1([[new]] : !llvm.float)
+  // CHECK-NEXT: llvm.cond_br [[ok]], ^bb2, ^bb1([[new]] : !llvm.i32)
   // CHECK-NEXT: ^bb2:
-  %c2 = constant 2.0 : f32
-  %add = addf %c2, %x : f32
-  return %add : f32
-  // CHECK-NEXT: [[c2:%.*]] = llvm.mlir.constant(2.000000e+00 : f32)
-  // CHECK-NEXT: [[add:%.*]] = llvm.fadd [[c2]], [[new]] : !llvm.float
+  %c2 = constant 2 : i32
+  %add = addi %c2, %x : i32
+  return %add : i32
+  // CHECK-NEXT: [[c2:%.*]] = llvm.mlir.constant(2 : i32)
+  // CHECK-NEXT: [[add:%.*]] = llvm.add [[c2]], [[new]] : !llvm.i32
   // CHECK-NEXT: llvm.return [[add]]
 }
 
 // -----
 
 // CHECK-LABEL: func @assume_alignment
 func @assume_alignment(%0 : memref<4x4xf16>) {
   // CHECK: %[[PTR:.*]] = llvm.extractvalue %[[MEMREF:.*]][1] : !llvm.struct<(ptr<half>, ptr<half>, i64, array<2 x i64>, array<2 x i64>)>

mlir/test/Dialect/LLVMIR/invalid.mlir

func @cmpxchg_expected_ptr(%f32_ptr : !llvm.ptr<float>, %f32 : !llvm.float) {
   // expected-error@+1 {{expected LLVM IR pointer type for operand #0}}
   %0 = "llvm.cmpxchg"(%f32, %f32, %f32) {success_ordering=2,failure_ordering=2} : (!llvm.float, !llvm.float, !llvm.float) -> !llvm.struct<(float, i1)>
   llvm.return
 }
 
 // -----
 
 // CHECK-LABEL: @cmpxchg_mismatched_operands
-func @cmpxchg_mismatched_operands(%f32_ptr : !llvm.ptr<float>, %i32 : !llvm.i32) {
+func @cmpxchg_mismatched_operands(%i64_ptr : !llvm.ptr<i64>, %i32 : !llvm.i32) {
   // expected-error@+1 {{expected LLVM IR element type for operand #0 to match type for all other operands}}
-  %0 = "llvm.cmpxchg"(%f32_ptr, %i32, %i32) {success_ordering=2,failure_ordering=2} : (!llvm.ptr<float>, !llvm.i32, !llvm.i32) -> !llvm.struct<(i32, i1)>
+  %0 = "llvm.cmpxchg"(%i64_ptr, %i32, %i32) {success_ordering=2,failure_ordering=2} : (!llvm.ptr<i64>, !llvm.i32, !llvm.i32) -> !llvm.struct<(i32, i1)>
   llvm.return
 }
 
 // -----
 
 // CHECK-LABEL: @cmpxchg_unexpected_type
 func @cmpxchg_unexpected_type(%i1_ptr : !llvm.ptr<i1>, %i1 : !llvm.i1) {
   // expected-error@+1 {{unexpected LLVM IR type}}

mlir/test/Dialect/LLVMIR/roundtrip.mlir

 // CHECK-LABEL: @atomicrmw
 func @atomicrmw(%ptr : !llvm.ptr<float>, %val : !llvm.float) {
   // CHECK: llvm.atomicrmw fadd %{{.*}}, %{{.*}} unordered : !llvm.float
   %0 = llvm.atomicrmw fadd %ptr, %val unordered : !llvm.float
   llvm.return
 }
 
 // CHECK-LABEL: @cmpxchg
-func @cmpxchg(%ptr : !llvm.ptr<float>, %cmp : !llvm.float, %new : !llvm.float) {
-  // CHECK: llvm.cmpxchg %{{.*}}, %{{.*}}, %{{.*}} acq_rel monotonic : !llvm.float
-  %0 = llvm.cmpxchg %ptr, %cmp, %new acq_rel monotonic : !llvm.float
+func @cmpxchg(%ptr : !llvm.ptr<i32>, %cmp : !llvm.i32, %new : !llvm.i32) {
+  // CHECK: llvm.cmpxchg %{{.*}}, %{{.*}}, %{{.*}} acq_rel monotonic : !llvm.i32
+  %0 = llvm.cmpxchg %ptr, %cmp, %new acq_rel monotonic : !llvm.i32
   llvm.return
 }
 
 llvm.mlir.global external constant @_ZTIi() : !llvm.ptr<i8>
 llvm.func @bar(!llvm.ptr<i8>, !llvm.ptr<i8>, !llvm.ptr<i8>)
 llvm.func @__gxx_personality_v0(...) -> !llvm.i32
 
 // CHECK-LABEL: @invokeLandingpad

mlir/test/Target/llvmir.mlir

llvm.func @atomicrmw(
   // CHECK: atomicrmw umax i32* %{{.*}}, i32 %{{.*}} unordered
   %11 = llvm.atomicrmw umax %i32_ptr, %i32 unordered : !llvm.i32
   // CHECK: atomicrmw umin i32* %{{.*}}, i32 %{{.*}} unordered
   %12 = llvm.atomicrmw umin %i32_ptr, %i32 unordered : !llvm.i32
   llvm.return
 }
 
 // CHECK-LABEL: @cmpxchg
-llvm.func @cmpxchg(%ptr : !llvm.ptr<float>, %cmp : !llvm.float, %val: !llvm.float) {
-  // CHECK: cmpxchg float* %{{.*}}, float %{{.*}}, float %{{.*}} acq_rel monotonic
-  %0 = llvm.cmpxchg %ptr, %cmp, %val acq_rel monotonic : !llvm.float
-  // CHECK: %{{[0-9]+}} = extractvalue { float, i1 } %{{[0-9]+}}, 0
-  %1 = llvm.extractvalue %0[0] : !llvm.struct<(float, i1)>
-  // CHECK: %{{[0-9]+}} = extractvalue { float, i1 } %{{[0-9]+}}, 1
-  %2 = llvm.extractvalue %0[1] : !llvm.struct<(float, i1)>
+llvm.func @cmpxchg(%ptr : !llvm.ptr<i32>, %cmp : !llvm.i32, %val: !llvm.i32) {
+  // CHECK: cmpxchg i32* %{{.*}}, i32 %{{.*}}, i32 %{{.*}} acq_rel monotonic
+  %0 = llvm.cmpxchg %ptr, %cmp, %val acq_rel monotonic : !llvm.i32
+  // CHECK: %{{[0-9]+}} = extractvalue { i32, i1 } %{{[0-9]+}}, 0
+  %1 = llvm.extractvalue %0[0] : !llvm.struct<(i32, i1)>
+  // CHECK: %{{[0-9]+}} = extractvalue { i32, i1 } %{{[0-9]+}}, 1
+  %2 = llvm.extractvalue %0[1] : !llvm.struct<(i32, i1)>
   llvm.return
 }
 
 llvm.mlir.global external constant @_ZTIi() : !llvm.ptr<i8>
 llvm.func @foo(!llvm.ptr<i8>)
 llvm.func @bar(!llvm.ptr<i8>) -> !llvm.ptr<i8>
 llvm.func @__gxx_personality_v0(...) -> !llvm.i32