Page MenuHomePhabricator

[mlir][llvm] allow mlir-translate carry custom triple and data layout.
Needs ReviewPublic

Authored by whchung on Apr 28 2020, 9:37 AM.

Details

Summary
  • Teach mlir-translate to use custom triple and data layout.
  • Change convert-to-rocdlir pass to pass AMDGPU-specific triple and target layout string.
  • Amend test case to check alloca on non-zero addrspace.

Diff Detail

Unit TestsFailed

TimeTest
20 msLLVM.tools/llvm-xray/X86::unsupported-elf32.txt
Script: -- : 'RUN: at line 1'; not /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/llvm-xray extract /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/tools/llvm-xray/X86/Inputs/elf32-noxray.bin 2>&1 | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/tools/llvm-xray/X86/unsupported-elf32.txt
60 msLLVM.tools/llvm-xray/X86::unsupported-elf32.txt
Script: -- : 'RUN: at line 1'; not c:\ws\prod\llvm-project\build\bin\llvm-xray.exe extract C:\ws\prod\llvm-project\llvm\test\tools\llvm-xray\X86/Inputs/elf32-noxray.bin 2>&1 | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\llvm\test\tools\llvm-xray\X86\unsupported-elf32.txt

Event Timeline

whchung created this revision.Apr 28 2020, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 9:37 AM
whchung added a project: Restricted Project.Apr 28 2020, 9:43 AM
ftynse accepted this revision.Apr 28 2020, 10:21 AM
ftynse added a subscriber: aartbik.

Thanks, I have been discussing a similar change with @aartbik just today. Please add the test on translation and feel free to land.

As an improvement (fine for a follow-up), could you please expose the triple and data layout as mlir-translate command line options?

mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
82

Could you add a test that the LLVM module indeed has the specified triple and target and put it under test/Target ?

This revision is now accepted and ready to land.Apr 28 2020, 10:21 AM
whchung updated this revision to Diff 260695.Apr 28 2020, 10:44 AM

Check triple and data layout string.

whchung marked an inline comment as done.Apr 28 2020, 10:46 AM

Thanks, I have been discussing a similar change with @aartbik just today. Please add the test on translation and feel free to land.

As an improvement (fine for a follow-up), could you please expose the triple and data layout as mlir-translate command line options?

@ftynse I just revised the test. Will submit a follow-up patch for mlir-translate command line options. I don't have authority to land the patch though.

@ftynse I used to submit PRs to MLIR on GitHub. It's actually my first patch on Phabricator so I don't have access push to master. Would you mind help me make the commit? Thanks a lot.

I've got several other patches under review right now. Once I'm more acquainted with the process I'll request write access.

mehdi_amini added inline comments.Apr 28 2020, 4:55 PM
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
61

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.

mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

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?

whchung updated this revision to Diff 260823.Apr 28 2020, 7:43 PM

Address some review comments.

whchung marked 2 inline comments as done.Apr 28 2020, 7:54 PM
whchung added inline comments.
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
61

@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.

mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

@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.

mehdi_amini added inline comments.Apr 28 2020, 8:32 PM
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
61

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?

mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

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 requested changes to this revision.Apr 28 2020, 8:33 PM
This revision now requires changes to proceed.Apr 28 2020, 8:33 PM
whchung marked 3 inline comments as done.Apr 28 2020, 9:50 PM
whchung added inline comments.
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
61

@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.

mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

@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 .

mehdi_amini added inline comments.Apr 28 2020, 10:20 PM
mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

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?

84

Unless: if we don't provide one and provide only a triple we get the default one for the triple?

whchung marked 2 inline comments as done.Apr 28 2020, 11:13 PM
whchung added inline comments.
mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

@mehdi_amini

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

  1. create a TargetMachine from a triple
  2. use TargetMachine::createDataLayout to fetch the data layout string associated with the triple
  3. 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 .

ftynse added inline comments.Apr 29 2020, 7:01 AM
mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

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.

mehdi_amini added inline comments.Apr 29 2020, 11:40 PM
mlir/lib/Target/LLVMIR/ConvertToROCDLIR.cpp
84

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"?

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.

That is why I recommended that mlir-translate take the triple and the data layout if necessary, so it can derive the necessary parts of the LLVM module. This is still dangerous, but will start the bottom-up progression of the data layout until we hit the right position for it in the MLIR.

Right now I see this as implementing something deliberately incorrect.

The flow we already have is also incorrect. I see this patch as less problematic as long as it does not claim it corrects the current flow.

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:

Sure, we agree here, let's do this.

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

I abuse the term a bit, but you know how it works in MLIR: any substantial discussion will be repeatedly side-tracked into considering an ever-increasing number of cases (or deciding they are not worth considering) and paralyze the progress here for a significant amount of time because analysis is happening somewhere.

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.

That is why I recommended that mlir-translate take the triple and the data layout if necessary, so it can derive the necessary parts of the LLVM module.

What do you see being "derived" on the LLVM module by mlir-translate with this information that wouldn't already be materialized in the LLVM dialect?

Right now I see this as implementing something deliberately incorrect.

The flow we already have is also incorrect. I see this patch as less problematic as long as it does not claim it corrects the current flow.

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:

Sure, we agree here, let's do this.

@whchung how does that sounds?

What do you see being "derived" on the LLVM module by mlir-translate with this information that wouldn't already be materialized in the LLVM dialect?

Data layout from the triple.

We should then move this earlier in the pipeline, data layout is actually necessary in std->llvm conversion to properly convert index, among others. So you are right that it should be materialized in the LLVM dialect.

whchung updated this revision to Diff 261420.Apr 30 2020, 7:07 PM

Address review comments from @mehdi_amini and @ftynse. Add TODO in the revised interface.

Right now I see this as implementing something deliberately incorrect.

The flow we already have is also incorrect. I see this patch as less problematic as long as it does not claim it corrects the current flow.

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:

Sure, we agree here, let's do this.

@whchung how does that sounds?

@mehdi_amini Agree. I've revised the patch so it has a TODO statement in the new interface. I'm applying for a new account on Bugzilla so if you could create one that would be very helpful.

@ftynse I agree with your assessment that data layout is best conveyed in std->llvm conversion process. I myself am seeing sub-optimal codes generated due to how index types get produced.