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

[Wen-Heng (Jack) Chung via Phabricator via llvm-commits]

whchung marked 2 inline comments as done.
whchung added inline comments.


================
Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:54
+  translateModule(Operation *m, StringRef triple = "",
+                  StringRef dataLayout = "") {
     if (!satisfiesLLVMModule(m))
----------------
mehdi_amini wrote:
> 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.
@mehdi_amini I'm not quite sure if introducing the concept of triple and data layout in MLIR is the right approach. 

Up until now, all dialects (including llvm dialect) do not require any special treatment for target triple or data layout. It's perfect fine to use `std.alloca`, or `llvm.alloca` to model allocating on-stack variables in std and llvm dialect. The only thing which is broken at this moment, is when we translate llvm dialect to llvm IR we couldn't get the proper instruction.


================
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>(
----------------
mehdi_amini wrote:
> 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?
@mehdi_amini I just changed the patch to use StringRef.

On most of the targets on LLVM there is an `computeDataLayout` function which get you the data layout string given a triple. But none of those functions are exposed as public functions. I want to restrict the scope of the patch to be within MLIR so I hard-coded the string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019