[PATCH] D136746: [mlir] Saturation arithmetic intrinsics

Wed Oct 26 01:59:52 PDT 2022

gysit added inline comments.

================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td:150-153
 }

+// Saturation Arithmetic Intrinsics.
+
+def LLVM_SAddSat
----------------
nit: There are quite a lot of changes in the diff for things that seemingly did not change (e.g. the overflow ops or the memove memset tests). Could it be your editor inserts tabs instead of spaces?

================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td:173
 //
 // Coroutine intrinsics.
 //
----------------
Nice! 

I have just landed a revision that makes the type constraints for llvm intrinsics more precise (https://reviews.llvm.org/D136360). Can you update your newly added intrinsics to have more accurate type constraints as well (e.g. AnySignlessInteger instead of just LLVM_Type)? I think they may be best defined by deriving from LLVM_BinarySameArgsIntrOpI since they take two arguments and return one result of the same type if I am not mistaken:

```
def LLVM_SMaxOp : LLVM_BinarySameArgsIntrOpI<"sadd.sat">;
def LLVM_SMaxOp : LLVM_BinarySameArgsIntrOpI<"ssub.sat">;
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136746