[PATCH] D74378: [MLIR] Add std.assume_align op.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.

Thanks for pushing on this. Having alignment attribute as an ODS argument looks like it would remove a bunch of boilerplate for you.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/Ops.td:1654
+
+  let arguments = (ins AnyMemRef:$memref);
+  let results = (outs);
----------------
Can you rather add alignment attribute as ODS argument, i.e. `(ins AnyMemRef:$memref, I32Attr align)` ? This will remove the need for a custom builder, and give you an auto-generated `align()` instead of hand-written `getAlignment()`.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2510
 
+struct AssumeAlignOpLowering : public LLVMLegalizationPattern<AssumeAlignOp> {
+  using LLVMLegalizationPattern<AssumeAlignOp>::LLVMLegalizationPattern;
----------------
rriddle wrote:
> Are these patterns sorted alphabetically/can they be?
They are not ordered, AFAIK. We should do it as a separate cleanup.


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:2772
+// assume_align `memref` { align = `alignment` } : memref_type
+static ParseResult parseAssumeAlignOp(OpAsmParser &parser,
+                                      OperationState &result) {
----------------
rriddle wrote:
> Please use the declarative assembly form instead:
> https://mlir.llvm.org/docs/OpDefinitions/#declarative-assembly-format
> 
> Should be something like:
> 
> ```
> let assemblyFormat = "$memref attr-dict `:` type($memref)";
> ```
Have you considered a non-dictionary syntax: `assume_align 64, %memref` ?  This should be also possible with declarative assembly.


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:2789
+static LogicalResult verify(AssumeAlignOp op) {
+  if (!op.getAttrOfType<IntegerAttr>(op.getAlignmentAttrName()))
+    return op.emitOpError("missing integer attribute `align`");
----------------
rriddle wrote:
> Why is the alignment not specified as an argument in ODS? That would autogenerate a lot of this for you automatically; builders, verification, etc.
I made the same comment above, +1!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74378