[PATCH] D75987: [mlir][AVX512] Start a primitive AVX512 dialect

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.h:12
+
+#include "mlir/Transforms/DialectConversion.h"
+
----------------
Can we trim this include, and replace with forward declare?


================
Comment at: mlir/include/mlir/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.h:24
+/// Create a pass to convert AVX512 operations to the LLVMIR dialect.
+OpPassBase<ModuleOp> *createConvertAVX512ToLLVMPass();
+
----------------
nit: Prefer std::unique_ptr<> here instead.


================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512.td:38
+    // Supports vector<16xf32> and vector<8xf64>.
+    Arguments<(ins VectorOfLengthAndType<[16, 8], [F32, F64]>:$src,
+                   I32:$k,
----------------
nit: Can you use `let arguments =` and `let results =` for these ops instead of using Arguments/Results? It is much cleaner IMO


================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512.td:62
+  let assemblyFormat =
+    // TODO(riverriddle, ntv): type($imm) should be dependent on type($dst).
+    "$src `,` $k `,` $a `,` $imm `,` $rounding attr-dict `:` type($dst) `,` type($imm)";
----------------
This should be possible now by using the TypesMatchWith constraint


================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512.td:93
+  let assemblyFormat =
+    // TODO(riverriddle, ntv): type($k) should be dependent on type($dst).
+    "$src `,` $a `,` $b `,` $k `,` $rounding attr-dict `:` type($dst) `,` type($k)";
----------------
Same here.


================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512Dialect.h:13
+
+#ifndef MLIR_DIALECT_LLVMIR_AVX512DIALECT_H_
+#define MLIR_DIALECT_LLVMIR_AVX512DIALECT_H_
----------------
This header guard seems wrong.


================
Comment at: mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp:45
+    packedType = typeConverter.packFunctionResults(op->getResultTypes());
+    if (!packedType) return lowering.matchFailure();
+  }
----------------
This doesn't look like it has been formatted.


================
Comment at: mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp:83
+      ConversionPatternRewriter &rewriter) const override {
+    if (!cast<MaskRndScaleOp>(op)
+             .src()
----------------
Can you hoist most of this so that we don't need to spread it across five lines?


================
Comment at: mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp:105
+      ConversionPatternRewriter &rewriter) const override {
+    if (!cast<MaskRndScaleOp>(op)
+             .src()
----------------
Same here.


================
Comment at: mlir/lib/Target/LLVMIR/AVX512Intr.cpp:30
+  LogicalResult convertOperation(Operation &opInst,
+                                 llvm::IRBuilder<> &builder) override {
+#include "mlir/Dialect/AVX512/AVX512Conversions.inc"
----------------
Not sure if this is possible yet, but we should really remove the need to use inheritance for extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75987