[PATCH] D134425: [NFC] Create a AllocLikeOpInterface and make memref::AllocOp, memref::AllocaOp and gpu::AllocOp implement it.

[Alex Zinenko via Phabricator via cfe-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

I would expect a new top-level interface (in lib/Interfaces) to go through the RFC process.

I have concerns about the interface design. Specifically, it appears to be promoting details specific to operations from one dialect (`memref.alloc(a)`) such as the fact that the result of an allocation is a memref (it's not, we have allocations in LLVM and SPIR-V that produce different types), that it has alignment defined as integer, and that it has some symbol operands presumably related to memref's affine layouts. If this were to live in `lib/Interfaces`, it should make either cover wider range of allocation-like operations or make a clear case as to why this is not desired accompanied by a name change.



================
Comment at: mlir/include/mlir/Dialect/GPU/IR/GPUOps.td:937-954
+
+    /// Returns the dynamic sizes for this alloc operation if specified.
+    operand_range getDynamicSizes() { return dynamicSizes(); }
+
+    /// Returns the symbol operands for this alloc operation if specified.
+    operand_range getSymbolOperands() { return symbolOperands(); }
+
----------------
I suppose these are only necessary because the dialect doesn't emit accessor using the prefixed form? Make sure to rebase this commit because the GPU dialect must have been switched to the prefixed form - https://discourse.llvm.org/t/psa-raw-accessors-are-being-removed/65629.


================
Comment at: mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td:103-120
+
+    /// Returns the dynamic sizes for this alloc operation if specified.
+    operand_range getDynamicSizes() { return dynamicSizes(); }
+
+    /// Returns the symbol operands for this alloc operation if specified.
+    operand_range getSymbolOperands() { return symbolOperands(); }
+
----------------
Ditto.


================
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.h:1
+//===- AllocLikeOpInterface.h - alloc like  operations interface-===//
+//
----------------
Please use the correct header line.


================
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:1
+//===-------------- AllocLikeOpInterface.td -*- tablegen -*--------------------===//
+//
----------------
Please follow the (implicit) convention from other files for this line: the file name is left aligned, the file type is right aligned, and the entire line fits into 80 cols.


================
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:9
+//
+// Defines the interface for copy-like operations.
+//
----------------
This looks wrong.


================
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:20
+  let description = [{
+    A alloc-like operation is one that allocates memory of a given type.
+  }];
----------------
Type is not mandatory for allocations, can be just bytes.


================
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:26
+    InterfaceMethod<
+      /*desc=*/"Returns the dynamic dimension sizes of the resultant memref.",
+      /*retTy=*/"::llvm::SmallVector<::mlir::Value>",
----------------
Memref is not a mandatory concept for allocating memory, I would rather not enshrine it in the top-level interface.


================
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:27
+      /*desc=*/"Returns the dynamic dimension sizes of the resultant memref.",
+      /*retTy=*/"::llvm::SmallVector<::mlir::Value>",
+      /*methodName=*/"getDynamicSizes"
----------------
Does it have to be a vector (copy) of values? Can't it be an `OperandRange`, a `ValueRange` or something similar?


================
Comment at: mlir/lib/Interfaces/AllocLikeOpInterface.cpp:1
+//===- AllocLikeOpInterface.cpp - Alloc like operations interface in MLIR-===//
+//
----------------
Nit: 80 cols.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134425