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

Tue Apr 28 22:38:49 PDT 2020

mehdi_amini added inline comments.

================
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:
> > 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.
> @mehdi_amini It's definitely possible to create a TargetMachine so we can get Triple and DataLayout. However `mlir-translate` is designed to produce LLVM IR, not machine ISA, so currently a TargetMachine is not constructed.
> 
> Would it be feasible if target triple and data layout be set as command-line options for `mlir-translate`? This approach was also suggested by @ftynse .
Have you looked into how do any frontend that wants to emit LLVM IR retrieve a data layout? Because they have to provide one right?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019