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

[Wen-Heng (Jack) Chung via Phabricator via llvm-commits]

whchung marked 3 inline comments as done.
whchung added inline comments.


================
Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:54
+  translateModule(Operation *m, StringRef triple = "",
+                  StringRef dataLayout = "") {
     if (!satisfiesLLVMModule(m))
----------------
mehdi_amini wrote:
> 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?
@mehdi_amini `alignment` is an optional attribute for `std.alloca` and `llvm.alloca`. So it's up to the IR builder to figure it out, when an alignment is not specified. And that would correspond to the topic how to properly specify Target Triple and Data Layout. And I think we can move this discussion to the next thread wrt should a TargetMachine be instantiated.


================
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:
> 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 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019