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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

What's the plan with separating ops taking std types from ops taking LLVM types? According to a comment, you want to do that, maybe do it immediately to avoid file moves and build system updates later.



================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512.td:65
+  // Fully specified by traits.
+  let verifier = ?;
+  let assemblyFormat =
----------------
It should be possible to just omit this


================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512.td:125
+def LLVM_x86_avx512_mask_scalef_ps_512 :
+  AVX512_IntrOp<"mask.scalef.ps.512">,
+  Arguments<(ins LLVM_Type, LLVM_Type, LLVM_Type, LLVM_Type, LLVM_Type)>;
----------------
Nit: can we use consistent formatting? Previous block has 4 leading spaces in exactly the same sitatuion.


================
Comment at: mlir/include/mlir/IR/OpImplementation.h:111
   }
-  template <typename TypeRange>
-  void printArrowTypeList(TypeRange &&types) {
+  template <typename TypeRange> void printArrowTypeList(TypeRange &&types) {
     auto &os = getStream() << " -> ";
----------------
Use git-clang-format to avoid spurious reformatting in future commits


================
Comment at: mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp:131
+    VectorType srcVectorType =
+        cast<MaskScaleFOp>(op).src().getType().cast<VectorType>();
+    if (!srcVectorType.getElementType().isF32())
----------------
It looks like you could have

```
template <typename OpTy>
Type getVectorElementType(OpTy op) {
  return op.src().getType().cast<VectorType>().getElementType();
}
```

that you could reuse in all four patterns as

```
if (getVectorElementType(cast<OpTy>(op)).isF32()) { ... }
```
```


================
Comment at: mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp:163
+  MLIRContext *ctx = converter.getDialect()->getContext();
+  patterns.insert<MaskRndScaleOpPS512Conversion, MaskRndScaleOpPD512Conversion,
+                  ScaleFOpPS512Conversion, ScaleFOpPD512Conversion>(ctx,
----------------
Nit: let's turn off clang-format here and just have one pattern per line. It reduces history churn due to reflowing lines every time one adds a new pattern in the list.


================
Comment at: mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp:185
+  target.addLegalDialect<LLVM::AVX512Dialect>();
+  // TODO(ntv): Ops in the td file need to be split to avoid adding the
+  // following manually.
----------------
We can go full metaprogramming and generate this function (and potentially the intrinsic-related ops as well)


================
Comment at: mlir/lib/Target/LLVMIR/AVX512Intr.cpp:30
+  LogicalResult convertOperation(Operation &opInst,
+                                 llvm::IRBuilder<> &builder) override {
+#include "mlir/Dialect/AVX512/AVX512Conversions.inc"
----------------
nicolasvasilache wrote:
> rriddle wrote:
> > Not sure if this is possible yet, but we should really remove the need to use inheritance for extension.
> I don't see any CRTP on ModuleTranslation yet, can we keep this as an NFC followup refactor?
@rriddle it is not possible yet, but it is on my todo list. I won't get to it in the next couple of weeks, so if somebody else has cycles, please go ahead :)


================
Comment at: mlir/tools/mlir-opt/CMakeLists.txt:8
+
+foreach (_variableName ${dialect_libs})
+    message(STATUS "${_variableName}")
----------------
Leftover debug?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75987