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

Thu Apr 30 15:06:53 PDT 2020

mehdi_amini added a comment.

In D79019#2012531 <https://reviews.llvm.org/D79019#2012531>, @ftynse wrote:

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019