[PATCH] D79019: [mlir][llvm] allow mlir-translate carry custom triple and data layout.

Tue Apr 28 17:18:10 PDT 2020

mehdi_amini added inline comments.

================
Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:54
+  translateModule(Operation *m, StringRef triple = "",
+                  StringRef dataLayout = "") {
     if (!satisfiesLLVMModule(m))
----------------
I am not convinced by this API: the datalayout is not something we should inject blindly after the fact. This is something that should be defined in MLIR in the first place, otherwise anything done before will make assumption about it.

================
Comment at: mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp:83
+      "v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:"
+      "1024-v2048:2048-n32:64-S32-A5-ni:7";
+  auto llvmModule = LLVM::ModuleTranslation::translateModule<ModuleTranslation>(
----------------
Please don't use auto here, use StringRef instead.

Also having these hard-coded here is a bit strange: can we get this from LLVM instead?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019