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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse 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:
> > 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 .
Correctness of transformations in MLIR wrt data layout is a separate topic, that needs to be handled within MLIR and likely requires non-trivial design, so I feel quite strongly about not blocking this diff on it. (1) We currently don't have any guarantee at all, so potentially _all_ transformations for _any_ layout are wrong. This patch does not make it any worse; if anything, it makes debugging easier because you see the layout in some cases. (2) MLIR translation is a front-end from LLVM's point of view. The front-end should specify a layout, how -- it's up to the front-end. This is no different from typing `data layout` in the textual module, or from doing casts with vector-type builtins in C.

My thinking is that we ultimately need "triple" and "layout" associated with MLIR's incarnation of the LLVM module. E.g., as attributes on the ModuleOp that comes out of *->LLVM dialect conversion. Then mlir-translate can pick it up transparently, and work in both directions. This requires some untangling in the llvm conversion passes, so in the meantime, we can have mlir-translate set the triple and layout.

Certainly, we can think about having a target description with a layout in MLIR itself, but while we are in the process of thinking, progress is necessary, lest we get stuck in the analysis paralysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019