From 994c9d7248e766455c28d0a31a210e500d52bd40 Mon Sep 17 00:00:00 2001 From: Fabian Mora <6982088+fabianmcg@users.noreply.github.com> Date: Sun, 27 Apr 2025 12:39:48 +0000 Subject: [PATCH 1/2] [mlir][ptr] Switch `LogicalResult` to `bool` in `MemorySpaceAttrInterface` This patch switches the return type in `MemorySpaceAttrInterface` methods from `LogicalResult` to `bool`. As `is*` methods are predicates. Users of the `MemorySpaceAttrInterface` API must note that, if `emitError` is non-null and the result of a `is*` method is `false`, then an error was likely emitted. To avoid the emission of an error the user can pass a default constructed `emitError`. --- .../Dialect/Ptr/IR/MemorySpaceInterfaces.td | 28 ++++++++++++----- mlir/lib/Dialect/Ptr/IR/PtrAttrs.cpp | 24 +++++++------- mlir/test/lib/Dialect/Test/TestAttributes.cpp | 31 ++++++++++--------- 3 files changed, 48 insertions(+), 35 deletions(-) diff --git a/mlir/include/mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.td b/mlir/include/mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.td index 3e9f99e0e1d6a..54efeb0bc0f9e 100644 --- a/mlir/include/mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.td +++ b/mlir/include/mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.td @@ -35,8 +35,10 @@ def MemorySpaceAttrInterface : AttrInterface<"MemorySpaceAttrInterface"> { This method checks if it's valid to load a value from the memory space with a specific type, alignment, and atomic ordering. If `emitError` is non-null then the method is allowed to emit errors. + Furthermore, if `emitError` is non-null and the result is `false` an + error must have been emitted. }], - /*returnType=*/ "::mlir::LogicalResult", + /*returnType=*/ "bool", /*methodName=*/ "isValidLoad", /*args=*/ (ins "::mlir::Type":$type, "::mlir::ptr::AtomicOrdering":$ordering, @@ -48,8 +50,10 @@ def MemorySpaceAttrInterface : AttrInterface<"MemorySpaceAttrInterface"> { This method checks if it's valid to store a value in the memory space with a specific type, alignment, and atomic ordering. If `emitError` is non-null then the method is allowed to emit errors. + Furthermore, if `emitError` is non-null and the result is `false` an + error must have been emitted. }], - /*returnType=*/ "::mlir::LogicalResult", + /*returnType=*/ "bool", /*methodName=*/ "isValidStore", /*args=*/ (ins "::mlir::Type":$type, "::mlir::ptr::AtomicOrdering":$ordering, @@ -61,8 +65,10 @@ def MemorySpaceAttrInterface : AttrInterface<"MemorySpaceAttrInterface"> { This method checks if it's valid to perform an atomic operation in the memory space with a specific type, alignment, and atomic ordering. If `emitError` is non-null then the method is allowed to emit errors. + Furthermore, if `emitError` is non-null and the result is `false` an + error must have been emitted. }], - /*returnType=*/ "::mlir::LogicalResult", + /*returnType=*/ "bool", /*methodName=*/ "isValidAtomicOp", /*args=*/ (ins "::mlir::ptr::AtomicBinOp":$op, "::mlir::Type":$type, @@ -76,8 +82,10 @@ def MemorySpaceAttrInterface : AttrInterface<"MemorySpaceAttrInterface"> { in the memory space with a specific type, alignment, and atomic orderings. If `emitError` is non-null then the method is allowed to emit errors. + Furthermore, if `emitError` is non-null and the result is `false` an + error must have been emitted. }], - /*returnType=*/ "::mlir::LogicalResult", + /*returnType=*/ "bool", /*methodName=*/ "isValidAtomicXchg", /*args=*/ (ins "::mlir::Type":$type, "::mlir::ptr::AtomicOrdering":$successOrdering, @@ -90,8 +98,10 @@ def MemorySpaceAttrInterface : AttrInterface<"MemorySpaceAttrInterface"> { This method checks if it's valid to perform an `addrspacecast` op in the memory space. If `emitError` is non-null then the method is allowed to emit errors. + Furthermore, if `emitError` is non-null and the result is `false` an + error must have been emitted. }], - /*returnType=*/ "::mlir::LogicalResult", + /*returnType=*/ "bool", /*methodName=*/ "isValidAddrSpaceCast", /*args=*/ (ins "::mlir::Type":$tgt, "::mlir::Type":$src, @@ -101,11 +111,13 @@ def MemorySpaceAttrInterface : AttrInterface<"MemorySpaceAttrInterface"> { /*desc=*/ [{ This method checks if it's valid to perform a `ptrtoint` or `inttoptr` op in the memory space. - The first type is expected to be integer-like, while the second must be a - ptr-like type. + The first type is expected to be integer-like, while the second must be + a ptr-like type. If `emitError` is non-null then the method is allowed to emit errors. + Furthermore, if `emitError` is non-null and the result is `false` an + error must have been emitted. }], - /*returnType=*/ "::mlir::LogicalResult", + /*returnType=*/ "bool", /*methodName=*/ "isValidPtrIntCast", /*args=*/ (ins "::mlir::Type":$intLikeTy, "::mlir::Type":$ptrLikeTy, diff --git a/mlir/lib/Dialect/Ptr/IR/PtrAttrs.cpp b/mlir/lib/Dialect/Ptr/IR/PtrAttrs.cpp index 1770e4febf099..b819f56841852 100644 --- a/mlir/lib/Dialect/Ptr/IR/PtrAttrs.cpp +++ b/mlir/lib/Dialect/Ptr/IR/PtrAttrs.cpp @@ -23,45 +23,45 @@ constexpr const static unsigned kBitsInByte = 8; // GenericSpaceAttr //===----------------------------------------------------------------------===// -LogicalResult GenericSpaceAttr::isValidLoad( +bool GenericSpaceAttr::isValidLoad( Type type, ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return success(); + return true; } -LogicalResult GenericSpaceAttr::isValidStore( +bool GenericSpaceAttr::isValidStore( Type type, ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return success(); + return true; } -LogicalResult GenericSpaceAttr::isValidAtomicOp( +bool GenericSpaceAttr::isValidAtomicOp( ptr::AtomicBinOp op, Type type, ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return success(); + return true; } -LogicalResult GenericSpaceAttr::isValidAtomicXchg( +bool GenericSpaceAttr::isValidAtomicXchg( Type type, ptr::AtomicOrdering successOrdering, ptr::AtomicOrdering failureOrdering, IntegerAttr alignment, function_ref emitError) const { - return success(); + return true; } -LogicalResult GenericSpaceAttr::isValidAddrSpaceCast( +bool GenericSpaceAttr::isValidAddrSpaceCast( Type tgt, Type src, function_ref emitError) const { // TODO: update this method once the `addrspace_cast` op is added to the // dialect. assert(false && "unimplemented, see TODO in the source."); - return failure(); + return false; } -LogicalResult GenericSpaceAttr::isValidPtrIntCast( +bool GenericSpaceAttr::isValidPtrIntCast( Type intLikeTy, Type ptrLikeTy, function_ref emitError) const { // TODO: update this method once the int-cast ops are added to the dialect. assert(false && "unimplemented, see TODO in the source."); - return failure(); + return false; } //===----------------------------------------------------------------------===// diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp index 057d9fb4a215f..3524e85c2d234 100644 --- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp +++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp @@ -331,43 +331,44 @@ TestOpAsmAttrInterfaceAttr::getAlias(::llvm::raw_ostream &os) const { // TestConstMemorySpaceAttr //===----------------------------------------------------------------------===// -LogicalResult TestConstMemorySpaceAttr::isValidLoad( +bool TestConstMemorySpaceAttr::isValidLoad( Type type, mlir::ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return success(); + return true; } -LogicalResult TestConstMemorySpaceAttr::isValidStore( +bool TestConstMemorySpaceAttr::isValidStore( Type type, mlir::ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return emitError ? (emitError() << "memory space is read-only") : failure(); + return emitError ? failed(emitError() << "memory space is read-only") : false; } -LogicalResult TestConstMemorySpaceAttr::isValidAtomicOp( +bool TestConstMemorySpaceAttr::isValidAtomicOp( mlir::ptr::AtomicBinOp binOp, Type type, mlir::ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return emitError ? (emitError() << "memory space is read-only") : failure(); + return emitError ? failed(emitError() << "memory space is read-only") : false; } -LogicalResult TestConstMemorySpaceAttr::isValidAtomicXchg( +bool TestConstMemorySpaceAttr::isValidAtomicXchg( Type type, mlir::ptr::AtomicOrdering successOrdering, mlir::ptr::AtomicOrdering failureOrdering, IntegerAttr alignment, function_ref emitError) const { - return emitError ? (emitError() << "memory space is read-only") : failure(); + return emitError ? failed(emitError() << "memory space is read-only") : false; } -LogicalResult TestConstMemorySpaceAttr::isValidAddrSpaceCast( +bool TestConstMemorySpaceAttr::isValidAddrSpaceCast( Type tgt, Type src, function_ref emitError) const { - return emitError - ? (emitError() << "memory space doesn't allow addrspace casts") - : failure(); + return emitError ? failed(emitError() + << "memory space doesn't allow addrspace casts") + : false; } -LogicalResult TestConstMemorySpaceAttr::isValidPtrIntCast( +bool TestConstMemorySpaceAttr::isValidPtrIntCast( Type intLikeTy, Type ptrLikeTy, function_ref emitError) const { - return emitError ? (emitError() << "memory space doesn't allow int-ptr casts") - : failure(); + return emitError + ? failed(emitError() << "memory space doesn't allow int-ptr casts") + : false; } //===----------------------------------------------------------------------===// From 6cd11af54c5690915041a3dd2b3a155f68995452 Mon Sep 17 00:00:00 2001 From: Fabian Mora <6982088+fabianmcg@users.noreply.github.com> Date: Sun, 27 Apr 2025 13:44:55 +0000 Subject: [PATCH 2/2] address reviewer comments --- mlir/test/lib/Dialect/Test/TestAttributes.cpp | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp index 3524e85c2d234..8a22e55c938c3 100644 --- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp +++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp @@ -340,35 +340,41 @@ bool TestConstMemorySpaceAttr::isValidLoad( bool TestConstMemorySpaceAttr::isValidStore( Type type, mlir::ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return emitError ? failed(emitError() << "memory space is read-only") : false; + if (emitError) + emitError() << "memory space is read-only"; + return false; } bool TestConstMemorySpaceAttr::isValidAtomicOp( mlir::ptr::AtomicBinOp binOp, Type type, mlir::ptr::AtomicOrdering ordering, IntegerAttr alignment, function_ref emitError) const { - return emitError ? failed(emitError() << "memory space is read-only") : false; + if (emitError) + emitError() << "memory space is read-only"; + return false; } bool TestConstMemorySpaceAttr::isValidAtomicXchg( Type type, mlir::ptr::AtomicOrdering successOrdering, mlir::ptr::AtomicOrdering failureOrdering, IntegerAttr alignment, function_ref emitError) const { - return emitError ? failed(emitError() << "memory space is read-only") : false; + if (emitError) + emitError() << "memory space is read-only"; + return false; } bool TestConstMemorySpaceAttr::isValidAddrSpaceCast( Type tgt, Type src, function_ref emitError) const { - return emitError ? failed(emitError() - << "memory space doesn't allow addrspace casts") - : false; + if (emitError) + emitError() << "memory space doesn't allow addrspace casts"; + return false; } bool TestConstMemorySpaceAttr::isValidPtrIntCast( Type intLikeTy, Type ptrLikeTy, function_ref emitError) const { - return emitError - ? failed(emitError() << "memory space doesn't allow int-ptr casts") - : false; + if (emitError) + emitError() << "memory space doesn't allow int-ptr casts"; + return false; } //===----------------------------------------------------------------------===//