[mlir][Ptr] Add the MemorySpaceAttrInterface interface and dependencies.
#86870
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-mlir Author: Fabian Mora (fabianmcg) ChangesThis patch introduces the For example, this interface can be used to create read-only memory spaces, making any other operation other than a load a verification error, see This patch also introduces Enum depedencies Also, see:
Note: Ignore the first commit, that's being reviewed in #86860 . Patch is 48.04 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86870.diff 23 Files Affected:
diff --git a/mlir/include/mlir/Dialect/CMakeLists.txt b/mlir/include/mlir/Dialect/CMakeLists.txt
index 2da79011fa26a3..5f0e9806926145 100644
--- a/mlir/include/mlir/Dialect/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/CMakeLists.txt
@@ -28,6 +28,7 @@ add_subdirectory(OpenACCMPCommon)
add_subdirectory(OpenMP)
add_subdirectory(PDL)
add_subdirectory(PDLInterp)
+add_subdirectory(Ptr)
add_subdirectory(Quant)
add_subdirectory(SCF)
add_subdirectory(Shape)
diff --git a/mlir/include/mlir/Dialect/Ptr/CMakeLists.txt b/mlir/include/mlir/Dialect/Ptr/CMakeLists.txt
new file mode 100644
index 00000000000000..f33061b2d87cff
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Ptr/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(IR)
diff --git a/mlir/include/mlir/Dialect/Ptr/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Ptr/IR/CMakeLists.txt
new file mode 100644
index 00000000000000..80dffdfd402cf2
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Ptr/IR/CMakeLists.txt
@@ -0,0 +1,14 @@
+add_mlir_dialect(PtrOps ptr)
+add_mlir_doc(PtrOps PtrOps Dialects/ -gen-op-doc)
+
+set(LLVM_TARGET_DEFINITIONS MemorySpaceInterfaces.td)
+mlir_tablegen(MemorySpaceInterfaces.h.inc -gen-op-interface-decls)
+mlir_tablegen(MemorySpaceInterfaces.cpp.inc -gen-op-interface-defs)
+mlir_tablegen(MemorySpaceAttrInterfaces.h.inc -gen-attr-interface-decls)
+mlir_tablegen(MemorySpaceAttrInterfaces.cpp.inc -gen-attr-interface-defs)
+add_public_tablegen_target(MLIRPtrMemorySpaceInterfacesIncGen)
+
+set(LLVM_TARGET_DEFINITIONS PtrOps.td)
+mlir_tablegen(PtrOpsEnums.h.inc -gen-enum-decls)
+mlir_tablegen(PtrOpsEnums.cpp.inc -gen-enum-defs)
+add_public_tablegen_target(MLIRPtrOpsEnumsGen)
diff --git a/mlir/include/mlir/Dialect/Ptr/IR/MemorySpace.h b/mlir/include/mlir/Dialect/Ptr/IR/MemorySpace.h
new file mode 100644
index 00000000000000..e467d121f2c886
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Ptr/IR/MemorySpace.h
@@ -0,0 +1,161 @@
+//===-- MemorySpace.h - ptr dialect memory space ---------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the ptr's dialect memory space class and related
+// interfaces.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_DIALECT_PTR_IR_MEMORYSPACE_H
+#define MLIR_DIALECT_PTR_IR_MEMORYSPACE_H
+
+#include "mlir/IR/Attributes.h"
+#include "mlir/IR/BuiltinAttributes.h"
+#include "mlir/IR/OpDefinition.h"
+
+namespace mlir {
+class Operation;
+namespace ptr {
+/// This method checks if it's valid to perform an `addrspacecast` op in the
+/// memory space.
+/// Compatible types are:
+/// Vectors of rank 1, or scalars of `ptr` type.
+LogicalResult isValidAddrSpaceCastImpl(Type tgt, Type src,
+ Operation *diagnosticOp);
+
+/// This method checks if it's valid to perform a `ptrtoint` or `inttoptr` op in
+/// the memory space.
+/// Compatible types are:
+/// IntLikeTy: Vectors of rank 1, or scalars of integer types or `index` type.
+/// PtrLikeTy: Vectors of rank 1, or scalars of `ptr` type.
+LogicalResult isValidPtrIntCastImpl(Type intLikeTy, Type ptrLikeTy,
+ Operation *diagnosticOp);
+
+enum class AtomicBinOp : uint64_t;
+enum class AtomicOrdering : uint64_t;
+} // namespace ptr
+} // namespace mlir
+
+#include "mlir/Dialect/Ptr/IR/MemorySpaceAttrInterfaces.h.inc"
+
+namespace mlir {
+namespace ptr {
+/// This class wraps the `MemorySpaceAttrInterface` interface, providing a safe
+/// mechanism to specify the default behavior assumed by the ptr dialect.
+class MemorySpace {
+public:
+ MemorySpace() = default;
+ MemorySpace(std::nullptr_t) {}
+ MemorySpace(MemorySpaceAttrInterface memorySpace)
+ : memorySpaceAttr(memorySpace), memorySpace(memorySpace) {}
+ MemorySpace(Attribute memorySpace)
+ : memorySpaceAttr(memorySpace),
+ memorySpace(dyn_cast_or_null<MemorySpaceAttrInterface>(memorySpace)) {}
+
+ operator Attribute() const { return memorySpaceAttr; }
+ operator MemorySpaceAttrInterface() const { return memorySpace; }
+ bool operator==(const MemorySpace &memSpace) const {
+ return memSpace.memorySpaceAttr == memorySpaceAttr;
+ }
+
+ /// Returns the underlying memory space.
+ Attribute getUnderlyingSpace() const { return memorySpaceAttr; }
+
+ /// Returns true if the underlying memory space is null.
+ bool isDefaultModel() const { return memorySpace == nullptr; }
+
+ /// Returns the memory space as an integer, or 0 if using the default space.
+ unsigned getAddressSpace() const {
+ if (memorySpace)
+ return memorySpace.getAddressSpace();
+ if (auto intAttr = llvm::dyn_cast_or_null<IntegerAttr>(memorySpaceAttr))
+ return intAttr.getInt();
+ return 0;
+ }
+
+ /// Returns the default memory space as an attribute, or nullptr if using the
+ /// default model.
+ Attribute getDefaultMemorySpace() const {
+ return memorySpace ? memorySpace.getDefaultMemorySpace() : nullptr;
+ }
+
+ /// This method checks if it's valid to load a value from the memory space
+ /// with a specific type, alignment, and atomic ordering. The default model
+ /// assumes all values are loadable.
+ LogicalResult isValidLoad(Type type, AtomicOrdering ordering,
+ IntegerAttr alignment,
+ Operation *diagnosticOp = nullptr) const {
+ return memorySpace ? memorySpace.isValidLoad(type, ordering, alignment,
+ diagnosticOp)
+ : success();
+ }
+
+ /// This method checks if it's valid to store a value in the memory space with
+ /// a specific type, alignment, and atomic ordering. The default model assumes
+ /// all values are loadable.
+ LogicalResult isValidStore(Type type, AtomicOrdering ordering,
+ IntegerAttr alignment,
+ Operation *diagnosticOp = nullptr) const {
+ return memorySpace ? memorySpace.isValidStore(type, ordering, alignment,
+ diagnosticOp)
+ : success();
+ }
+
+ /// This method checks if it's valid to perform an atomic operation in the
+ /// memory space with a specific type, alignment, and atomic ordering.
+ LogicalResult isValidAtomicOp(AtomicBinOp op, Type type,
+ AtomicOrdering ordering, IntegerAttr alignment,
+ Operation *diagnosticOp = nullptr) const {
+ return memorySpace ? memorySpace.isValidAtomicOp(op, type, ordering,
+ alignment, diagnosticOp)
+ : success();
+ }
+
+ /// This method checks if it's valid to perform an atomic operation in the
+ /// memory space with a specific type, alignment, and atomic ordering.
+ LogicalResult isValidAtomicXchg(Type type, AtomicOrdering successOrdering,
+ AtomicOrdering failureOrdering,
+ IntegerAttr alignment,
+ Operation *diagnosticOp = nullptr) const {
+ return memorySpace ? memorySpace.isValidAtomicXchg(type, successOrdering,
+ failureOrdering,
+ alignment, diagnosticOp)
+ : success();
+ }
+
+ /// This method checks if it's valid to perform an `addrspacecast` op in the
+ /// memory space.
+ LogicalResult isValidAddrSpaceCast(Type tgt, Type src,
+ Operation *diagnosticOp = nullptr) const {
+ return memorySpace
+ ? memorySpace.isValidAddrSpaceCast(tgt, src, diagnosticOp)
+ : isValidAddrSpaceCastImpl(tgt, src, diagnosticOp);
+ }
+
+ /// This method checks if it's valid to perform a `ptrtoint` or `inttoptr` op
+ /// in the memory space.
+ LogicalResult isValidPtrIntCast(Type intLikeTy, Type ptrLikeTy,
+ Operation *diagnosticOp = nullptr) const {
+ return memorySpace
+ ? memorySpace.isValidPtrIntCast(intLikeTy, ptrLikeTy,
+ diagnosticOp)
+ : isValidPtrIntCastImpl(intLikeTy, ptrLikeTy, diagnosticOp);
+ }
+
+protected:
+ /// Underlying memory space.
+ Attribute memorySpaceAttr{};
+ /// Memory space.
+ MemorySpaceAttrInterface memorySpace{};
+};
+} // namespace ptr
+} // namespace mlir
+
+#include "mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.h.inc"
+
+#endif // MLIR_DIALECT_PTR_IR_MEMORYSPACE_H
diff --git a/mlir/include/mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.td b/mlir/include/mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.td
new file mode 100644
index 00000000000000..b7bed95434839e
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Ptr/IR/MemorySpaceInterfaces.td
@@ -0,0 +1,182 @@
+//===-- MemorySpaceInterfaces.td - Memory space interfaces ----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines memory space attribute interfaces.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef PTR_MEMORYSPACEINTERFACES
+#define PTR_MEMORYSPACEINTERFACES
+
+include "mlir/IR/AttrTypeBase.td"
+include "mlir/IR/OpBase.td"
+
+//===----------------------------------------------------------------------===//
+// Memory space attribute interface.
+//===----------------------------------------------------------------------===//
+
+def MemorySpaceAttrInterface : AttrInterface<"MemorySpaceAttrInterface"> {
+ let description = [{
+ This interface defines a common API for interacting with the memory model of
+ a memory space and the operations in the pointer dialect, giving proper
+ semantical meaning to the ops.
+
+ Furthermore, this interface allows concepts such as read-only memory to be
+ adequately modeled and enforced.
+ }];
+ let cppNamespace = "::mlir::ptr";
+ let methods = [
+ InterfaceMethod<
+ /*desc=*/ [{
+ Returns the dialect implementing the memory space.
+ }],
+ /*returnType=*/ "::mlir::Dialect*",
+ /*methodName=*/ "getMemorySpaceDialect",
+ /*args=*/ (ins),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{ return nullptr; }]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ Returns the default memory space as an attribute.
+ }],
+ /*returnType=*/ "::mlir::Attribute",
+ /*methodName=*/ "getDefaultMemorySpace",
+ /*args=*/ (ins),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ Returns the memory space as an integer, or 0 if using the default model.
+ }],
+ /*returnType=*/ "unsigned",
+ /*methodName=*/ "getAddressSpace",
+ /*args=*/ (ins),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{ return 0; }]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ This method checks if it's valid to load a value from the memory space
+ with a specific type, alignment, and atomic ordering.
+ If `diagnosticOp` is non-null then the method might emit diagnostics.
+ }],
+ /*returnType=*/ "::mlir::LogicalResult",
+ /*methodName=*/ "isValidLoad",
+ /*args=*/ (ins "::mlir::Type":$type,
+ "::mlir::ptr::AtomicOrdering":$ordering,
+ "::mlir::IntegerAttr":$alignment,
+ "::mlir::Operation*":$diagnosticOp),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ This method checks if it's valid to store a value in the memory space
+ with a specific type, alignment, and atomic ordering.
+ If `diagnosticOp` is non-null then the method might emit diagnostics.
+ }],
+ /*returnType=*/ "::mlir::LogicalResult",
+ /*methodName=*/ "isValidStore",
+ /*args=*/ (ins "::mlir::Type":$type,
+ "::mlir::ptr::AtomicOrdering":$ordering,
+ "::mlir::IntegerAttr":$alignment,
+ "::mlir::Operation*":$diagnosticOp),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ 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 `diagnosticOp` is non-null then the method might emit diagnostics.
+ }],
+ /*returnType=*/ "::mlir::LogicalResult",
+ /*methodName=*/ "isValidAtomicOp",
+ /*args=*/ (ins "::mlir::ptr::AtomicBinOp":$op,
+ "::mlir::Type":$type,
+ "::mlir::ptr::AtomicOrdering":$ordering,
+ "::mlir::IntegerAttr":$alignment,
+ "::mlir::Operation*":$diagnosticOp),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ This method checks if it's valid to perform an atomic exchange operation
+ in the memory space with a specific type, alignment, and atomic
+ orderings.
+ If `diagnosticOp` is non-null then the method might emit diagnostics.
+ }],
+ /*returnType=*/ "::mlir::LogicalResult",
+ /*methodName=*/ "isValidAtomicXchg",
+ /*args=*/ (ins "::mlir::Type":$type,
+ "::mlir::ptr::AtomicOrdering":$successOrdering,
+ "::mlir::ptr::AtomicOrdering":$failureOrdering,
+ "::mlir::IntegerAttr":$alignment,
+ "::mlir::Operation*":$diagnosticOp),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ This method checks if it's valid to perform an `addrspacecast` op
+ in the memory space.
+ Both types are expected to be vectors of rank 1, or scalars of `ptr`
+ type.
+ If `diagnosticOp` is non-null then the method might emit diagnostics.
+ }],
+ /*returnType=*/ "::mlir::LogicalResult",
+ /*methodName=*/ "isValidAddrSpaceCast",
+ /*args=*/ (ins "::mlir::Type":$tgt,
+ "::mlir::Type":$src,
+ "::mlir::Operation*":$diagnosticOp),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ InterfaceMethod<
+ /*desc=*/ [{
+ This method checks if it's valid to perform a `ptrtoint` or `inttoptr`
+ op in the memory space. `CastValidity::InvalidSourceType` always refers
+ to the 'ptr-like' type and `CastValidity::InvalidTargetType` always
+ refers to the `int-like` type.
+ The first type is expected to be integer-like, while the second must be a
+ ptr-like type.
+ If `diagnosticOp` is non-null then the method might emit diagnostics.
+ }],
+ /*returnType=*/ "::mlir::LogicalResult",
+ /*methodName=*/ "isValidPtrIntCast",
+ /*args=*/ (ins "::mlir::Type":$intLikeTy,
+ "::mlir::Type":$ptrLikeTy,
+ "::mlir::Operation*":$diagnosticOp),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ ];
+}
+
+def MemorySpaceOpInterface : OpInterface<"MemorySpaceOpInterface"> {
+ let description = [{
+ An interface for operations with a memory space.
+ }];
+
+ let cppNamespace = "::mlir::ptr";
+
+ let methods = [
+ InterfaceMethod<
+ /*desc=*/ "Returns the memory space of the op.",
+ /*returnType=*/ "::mlir::ptr::MemorySpace",
+ /*methodName=*/ "getMemorySpace",
+ /*args=*/ (ins),
+ /*methodBody=*/ [{}],
+ /*defaultImpl=*/ [{}]
+ >,
+ ];
+}
+#endif // PTR_MEMORYSPACEINTERFACES
diff --git a/mlir/include/mlir/Dialect/Ptr/IR/PtrAttrs.h b/mlir/include/mlir/Dialect/Ptr/IR/PtrAttrs.h
new file mode 100644
index 00000000000000..e6aa7635919f6d
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Ptr/IR/PtrAttrs.h
@@ -0,0 +1,20 @@
+//===- PtrAttrs.h - Pointer dialect attributes ------------------*- C++ -*-===//
+//
+// This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file declares the Ptr dialect attributes.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_DIALECT_PTR_IR_PTRATTRS_H
+#define MLIR_DIALECT_PTR_IR_PTRATTRS_H
+
+#include "mlir/IR/OpImplementation.h"
+
+#include "mlir/Dialect/Ptr/IR/PtrOpsEnums.h.inc"
+
+#endif // MLIR_DIALECT_PTR_IR_PTRATTRS_H
diff --git a/mlir/include/mlir/Dialect/Ptr/IR/PtrDialect.h b/mlir/include/mlir/Dialect/Ptr/IR/PtrDialect.h
new file mode 100644
index 00000000000000..92f877c20dbf07
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Ptr/IR/PtrDialect.h
@@ -0,0 +1,20 @@
+//===- PtrDialect.h - Pointer dialect ---------------------------*- C++ -*-===//
+//
+// This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the Ptr dialect.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_DIALECT_PTR_IR_PTRDIALECT_H
+#define MLIR_DIALECT_PTR_IR_PTRDIALECT_H
+
+#include "mlir/IR/Dialect.h"
+
+#include "mlir/Dialect/Ptr/IR/PtrOpsDialect.h.inc"
+
+#endif // MLIR_DIALECT_PTR_IR_PTRDIALECT_H
diff --git a/mlir/include/mlir/Dialect/Ptr/IR/PtrDialect.td b/mlir/include/mlir/Dialect/Ptr/IR/PtrDialect.td
new file mode 100644
index 00000000000000..bffae6b1ad71bb
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Ptr/IR/PtrDialect.td
@@ -0,0 +1,83 @@
+//===- PtrDialect.td - Pointer dialect ---------------------*- tablegen -*-===//
+//
+// This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef PTR_DIALECT
+#define PTR_DIALECT
+
+include "mlir/Interfaces/DataLayoutInterfaces.td"
+include "mlir/IR/AttrTypeBase.td"
+include "mlir/IR/BuiltinTypeInterfaces.td"
+include "mlir/IR/OpBase.td"
+
+//===----------------------------------------------------------------------===//
+// Pointer dialect definition.
+//===----------------------------------------------------------------------===//
+
+def Ptr_Dialect : Dialect {
+ let name = "ptr";
+ let summary = "Pointer dialect";
+ let cppNamespace = "::mlir::ptr";
+ let useDefaultTypePrinterParser = 1;
+ let useDefaultAttributePrinterParser = 0;
+}
+
+//===----------------------------------------------------------------------===//
+// Pointer type definitions
+//===----------------------------------------------------------------------===//
+
+class Ptr_Type<string name, string typeMnemonic, list<Trait> traits = []>
+ : TypeDef<Ptr_Dialect, name, traits> {
+ let mnemonic = typeMnemonic;
+}
+
+def Ptr_PtrType : Ptr_Type<"Ptr", "ptr", [
+ MemRefElementTypeInterface,
+ DeclareTypeInterfaceMethods<DataLayoutTypeInterface, [
+ "areCompatible", "getIndexBitwidth", "verifyEntries"]>
+ ]> {
+ let summary = "pointer type";
+ let description = [{
+ The `ptr` type is an opaque pointer type. This type typically represents
+ a reference to an object in memory. Pointers are optionally parameterized
+ by a memory space.
+ Syntax:
+
+ ```mlir
+ pointer ::=...
[truncated]
|
Dinistro
reviewed
Mar 29, 2024
gysit
reviewed
Apr 1, 2024
How do we do that if you append more commits? |
My plan is: If I add new commits to this PR, it shouldn't be an issue as the new commits will be on top. Once, the dependent PR is approved and merged, I'll remove the first commit locally and then force push. |
I'd be curious where? I'm only doing manual stack PRs.
I just figured how to select multiple commits to see in the diff, should work. |
fabianmcg
force-pushed
the
pr-ptr-memory-interface
branch
from
161d1fc to
5042d6f
Compare
joker-eph
reviewed
May 31, 2024
fabianmcg
force-pushed
the
pr-ptr-memory-interface
branch
from
5042d6f to
8f1bf17
Compare
|
The last commit addressed reviewer comments, resolve merge conflicts, and more importantly, it made |
|
Ping for review. |
|
I'll try to allocate time for this tomorrow, currently afk. |
Dinistro
approved these changes
Aug 20, 2024
fabianmcg
force-pushed
the
pr-ptr-memory-interface
branch
from
f4d9696 to
832c41b
Compare
joker-eph
reviewed
Aug 31, 2024
joker-eph
reviewed
Feb 12, 2025
…cies. This patch introduces the `MemorySpaceAttrInterface` interface. This interface is responsible for handling the semantics of `ptr` operations. For example, this interface can be used to create read-only memory spaces, making any other operation other than a load a verification error, see `TestConstMemorySpaceAttr` for a possible implementation of this concept. This patch also introduces Enum depedencies `AtomicOrdering`, and `AtomicBinOp`, both enumerations are clones of the Enums with same name in the LLVM Dialect.
fabianmcg
force-pushed
the
pr-ptr-memory-interface
branch
from
0863d76 to
fd8f300
Compare
joker-eph
approved these changes
Mar 19, 2025
joker-eph
reviewed
Apr 23, 2025
| with a specific type, alignment, and atomic ordering. | ||
| If `emitError` is non-null then the method is allowed to emit errors. | ||
| }], | ||
| /*returnType=*/ "::mlir::LogicalResult", |
There was a problem hiding this comment.
@fabianmcg : nitpicking here but methods that starts with is are predicates and should return bool: LogicalResult is a about method that can fail instead. Can you update?
There was a problem hiding this comment.
I can update. But these methods can emit an error, remember they get a ::llvm::function_ref<::mlir::InFlightDiagnostic()> emitError parameter.
Admittedly, I would prefer these methods to return something like a SuccesOrDiagnostic, so users can have more control on how to handle the result. Or to decouple the diagnostic from the validity method.
There was a problem hiding this comment.
That's a good immediate option, I'll change to that. But I think we are missing something similar to llvm::Expected, like:
template <typename T>
class DiagnosticOr : public std::optional<T> {
operator LogicalResult () const;
Diagnostic takeDiagnostic();
void emitDiagnostic();
T& getValue();
private:
Diagnostic message;
};Which would allow users to decide how to handle the diagnostic.
There was a problem hiding this comment.
I think we're trying to be more efficient by avoiding formatting a diagnostic when the user does not want it.
There was a problem hiding this comment.
On success we shouldn't format a diagnostic, but I'm still not convinced about the approach.
I'll change to FailureOr<bool>, I'll revisit if I have a better idea.
There was a problem hiding this comment.
On success we shouldn't format a diagnostic
I'm talking about the failing case where the user of the API would just discard the diagnostic.
There was a problem hiding this comment.
I just remembered the emitError is always optional and the user of the API can opt to pass a default constructed object, in which case the method cannot produce errors. Therefore, it already handles the case of discarding the diagnostic in a efficient manner.
I'll switch to FailureOr<bool>.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch introduces the
MemorySpaceAttrInterfaceinterface. This interface is responsible for handling the semantics ofptroperations.For example, this interface can be used to create read-only memory spaces, making any other operation other than a load a verification error, see
TestConstMemorySpaceAttrfor a possible implementation of this concept.This patch also introduces Enum dependencies
AtomicOrdering, andAtomicBinOp, both enumerations are clones of the Enums with the same name in the LLVM Dialect.Also, see:
ptrdialect & modularizing ptr ops in the LLVM dialect for rationale.Note: Ignore the first commit, that's being reviewed in #86860 .