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

[Mehdi AMINI via Phabricator via llvm-commits]

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))
----------------
whchung wrote:
> 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.
> Up until now, all dialects (including llvm dialect) do not require any special treatment for target triple or data layout. 

Well I'm not sure the assumption made in here is always correct, for example on the alignment of everything. 
Since LLVM transformations actually need the datalayout to be correct, why can MLIR transformations be correct without a similar concept?


================
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>(
----------------
whchung wrote:
> 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.
I expect that a TargetMachine can be created from a Triple and in turn it exposes access to a DataLayout?

There has to be a public way to get this out of LLVM otherwise every frontend would duplicate this information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019