[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/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:
> mehdi_amini wrote:
> > 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?
> Unless: if we don't provide one and provide only a triple we get the default one for the triple?
@mehdi_amini

Most of the LLVM applications I know follow the idiom of:

# create a `TargetMachine` from a triple
# use `TargetMachine::createDataLayout` to fetch the data layout string associated with the triple
# use `Module::setDataLayout`

- For `clang`, `llvm::sys::getDefaultTargetTriple()` is used if nothing is specified, and a custom `-triple` command line option could be set for cross compilation. A TargetMachine would be created in the code generation process.

- For XLA, a TargetMachine is also created according to the target.

- Within MLIR, there are several utilities which would also create a TargetMachine : JitRunner, ExecutionEngine, ConvertKernelFuncToCubin.  Notice all of these utilities require a TargetMachine because target machine instructions are expected to be produced.

So indeed they create a TargetMachine, specify target triple and data layout, because machine instructions are expected.


On the other hand, the implementation of `mlir-translate` doesn't employ a TargetMachine. All of the IR coming out from `mlir-translate` right now, doesn't carry triple or data layout at all. This patch is the first attempt to allow an llvm Module coming out from `mlir-translate` to carry non-empty triple & data layout.

Would it be feasible if target triple and data layout be set as command-line options for `mlir-translate`, just like what's there for `opt`? This approach was also suggested by @ftynse .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019