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

Wed Apr 29 23:59:01 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>(
----------------
ftynse wrote:
> 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.
> 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.

Yes, but it does so by patching it after the fact, which as far as I know is not correct: the module has been generated with assumptions from a Datalayout (implicitly) and you're actually force-changing it.

The topic of the TargetMachine is independent, and I don't really get your point: if any LLVM frontend requires a TM to be able to set the DataLayout (I don't know if there is an alternative...) then I don't see why mlir-translate shouldn't do the same.

> 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, 

Right now I see this as implementing something deliberately incorrect.
I am not opposed to taking shortcuts, but I'd like these to be clearly identified, for example there should be a tracking bug and big TODO statement:

```
// TODO(pr1234): adding a datalayout after the fact is incorrect and we need to have it exposed
// from the beginning and propagated during the lowering to LLVM dialect instead.
```

> lest we get stuck in the analysis paralysis.

I don't see any paralysis here, unless you include "no one is working on it"?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019