[PATCH] D140985: [IR] Support importing modules with invalid data layouts.

[Jannik Silvanus via Phabricator via llvm-commits]

jsilvanus marked an inline comment as done.
jsilvanus added a comment.

Thanks for reviewing @nikic. Comments inline.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:384
+          DataLayoutCallback(M->getTargetTriple(), TentativeDLStr)) {
+    M->setDataLayout(*LayoutOverride);
+  } else {
----------------
nikic wrote:
> It would be better to go through `DataLayout::parse` here as well, and report without location. setDataLayout will cause a fatal error for invalid data layouts, I believe.
That's correct. I've changed it to always use `DataLayout::parse` and return an error instead of running into a fatal error.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4210
+    TentativeDataLayoutStr = llvm::UpgradeDataLayoutString(
+        TentativeDataLayoutStr, TheModule->getTargetTriple());
+
----------------
nikic wrote:
> High level question: Why is the existing UpgradeDataLayoutString mechanigm not sufficient?
In the case this is intended for (requiring natural alignment for `i8`), it is true that there is a straightforward fix of a broken data layout string: Just replace `i8`s alignment with natural alignment.

But depending on how `i8` is used in the module, this can break semantics, requiring nontrivial fixup (e.g. adding manual padding). This does not seem to be necessary for DXIL though.

I wanted to avoid auto-upgrade (silently?) breaking a module, and adding the manual padding as part of auto-upgrade seems excessive, at least given that we're not aware of any cases actually requiring it.

On the other hand, letting the user fix the layout string avoids that issue of auto-upgrade breaking a module. If the user is sure that `i8` alignment does not matter, the callback provides the option to fix the DL string.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140985