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

Wed Jan 11 07:52:55 PST 2023

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM

================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4210
+    TentativeDataLayoutStr = llvm::UpgradeDataLayoutString(
+        TentativeDataLayoutStr, TheModule->getTargetTriple());
+
----------------
jsilvanus wrote:
> 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.  
I see. If this is intended for manual user intervention, then that makes sense.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140985