Page MenuHomePhabricator

[mlir] Saturation arithmetic intrinsics
AcceptedPublic

Authored by omri123 on Oct 26 2022, 1:19 AM.

Details

Summary

Add the following saturation arithmetic llvm intrinsic ops: sadd.sat, uadd.sat, ssub.sat, usub.sat

Diff Detail

Event Timeline

omri123 created this revision.Oct 26 2022, 1:19 AM
omri123 requested review of this revision.Oct 26 2022, 1:19 AM
gysit added inline comments.Oct 26 2022, 1:59 AM
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">;
omri123 updated this revision to Diff 470775.Oct 26 2022, 4:10 AM

Thanks for your comments! In this revision I fixed the diff and updated the intrinsics to use LLVM_BinarySameArgsIntrOpI.

gysit added a comment.Oct 26 2022, 4:24 AM

Looks good from my side modulo the nit comment. Let's wait for @ftynse (the code owner) for the final approval.

mlir/test/Target/LLVMIR/Import/intrinsic.ll
778

nit: the #0 seems unnecessary.

gysit added inline comments.Oct 26 2022, 4:26 AM
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.

ftynse added inline comments.Oct 26 2022, 4:32 AM
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.

omri123 updated this revision to Diff 470780.Oct 26 2022, 4:46 AM

Thanks again! In this revision I fixed both issues mentioned above (removed #0 from tests and renamed the tblgen definitions)

ftynse accepted this revision.Oct 26 2022, 4:58 AM

Please wait for the tests to pass before submitting.

This revision is now accepted and ready to land.Oct 26 2022, 4:58 AM

Thanks! Submitting.