[PATCH] D136746: [mlir] Saturation arithmetic intrinsics

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td:179
+def LLVM_SAddSat : LLVM_BinarySameArgsIntrOpI<"sadd.sat">;
+def LLVM_SAddSat : LLVM_BinarySameArgsIntrOpI<"uadd.sat">;
+def LLVM_SAddSat : LLVM_BinarySameArgsIntrOpI<"ssub.sat">;
----------------
gysit wrote:
> Ah I missed that one. The Op names need to differ of course LLVM_SAddSat, LLVM_UAddSat, etc.
Indeed, this is breaking the tests.


================
Comment at: mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir:809-816
+// CHECK-DAG: declare i32 @llvm.sadd.sat.i32(i32, i32) #0
+// CHECK-DAG: declare <8 x i32> @llvm.sadd.sat.v8i32(<8 x i32>, <8 x i32>) #0
+// CHECK-DAG: declare i32 @llvm.uadd.sat.i32(i32, i32) #0
+// CHECK-DAG: declare <8 x i32> @llvm.uadd.sat.v8i32(<8 x i32>, <8 x i32>) #0
+// CHECK-DAG: declare i32 @llvm.ssub.sat.i32(i32, i32) #0
+// CHECK-DAG: declare <8 x i32> @llvm.ssub.sat.v8i32(<8 x i32>, <8 x i32>) #0
+// CHECK-DAG: declare i32 @llvm.usub.sat.i32(i32, i32) #0
----------------
Nit: I'd rather remove the `#0` matadata because the `0` is transient. I see some cases above also match for that, but it is a mistake that makes tests fragile.


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

https://reviews.llvm.org/D136746