Add the following saturation arithmetic llvm intrinsic ops: sadd.sat, uadd.sat, ssub.sat, usub.sat
Details
Diff Detail
Event Timeline
mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td | ||
---|---|---|
162–163 | 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? | |
187 | 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">; |
Thanks for your comments! In this revision I fixed the diff and updated the intrinsics to use LLVM_BinarySameArgsIntrOpI.
mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td | ||
---|---|---|
179 | Ah I missed that one. The Op names need to differ of course LLVM_SAddSat, LLVM_UAddSat, etc. |
mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td | ||
---|---|---|
179 | Indeed, this is breaking the tests. | |
mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir | ||
809–816 | 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. |
Thanks again! In this revision I fixed both issues mentioned above (removed #0 from tests and renamed the tblgen definitions)
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?