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

Alex Zinenko via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 30 05:49:03 PDT 2020


ftynse added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79019





More information about the llvm-commits mailing list