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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2510
 
+struct AssumeAlignOpLowering : public LLVMLegalizationPattern<AssumeAlignOp> {
+  using LLVMLegalizationPattern<AssumeAlignOp>::LLVMLegalizationPattern;
----------------
Are these patterns sorted alphabetically/can they be?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2539
+            op->getLoc(), LLVM::ICmpPredicate::eq,
+            rewriter.create<LLVM::AndOp>(op->getLoc(),
+                                         rewriter.create<LLVM::PtrToIntOp>(
----------------
nit: Move some of these to temporary variables. The nesting is a bit awkward.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2665
       StoreOpLowering,
+      AssumeAlignOpLowering,
       SubViewOpLowering,
----------------
This breaks the ordering.


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:2772
+// assume_align `memref` { align = `alignment` } : memref_type
+static ParseResult parseAssumeAlignOp(OpAsmParser &parser,
+                                      OperationState &result) {
----------------
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)";
```


================
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`");
----------------
Why is the alignment not specified as an argument in ODS? That would autogenerate a lot of this for you automatically; builders, verification, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74378