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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512.td:16
+
+include "mlir/Dialect/LLVMIR/LLVMOpBase.td" // Needed for LLVM conversion
+include "mlir/Interfaces/SideEffects.td"
----------------
Is this still necessary?


================
Comment at: mlir/include/mlir/Dialect/AVX512/AVX512.td:44
+    The mask.rndscale is an AVX512 specific op that can lower to the proper
+    `llvm::Intrinsic::x86_avx512_mask_rndscale_ps_512` or
+    `llvm::Intrinsic::x86_avx512_mask_rndscale_pd_512` instruction depending on
----------------
Nit: I think it lowers to MLIR dialect operations rather than llvm intrinsics


================
Comment at: mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp:178
+  target.addLegalDialect<LLVM::LLVMAVX512Dialect>();
+  // TODO(ntv): Ops in the td file need to be split to avoid adding the
+  // following manually.
----------------
I think you can now do `addIllegalDialect<AVX512Dialect>`


================
Comment at: mlir/lib/Target/LLVMIR/LLVMAVX512Intr.cpp:44
+static TranslateFromMLIRRegistration
+    reg("avx512-mlir-to-llvmir", [](ModuleOp module, raw_ostream &output) {
+      auto llvmModule = translateLLVMAVX512ModuleToLLVMIR(module);
----------------
I don't see a problem including this into the "main" mlir to llvm translation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75987