[PATCH] D67631: Add AutoUpgrade function to add new address space datalayout string to existing datalayouts.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3583
         return error("Invalid record");
+      S = llvm::UpgradeDataLayoutString(S, TheModule->getTargetTriple());
       TheModule->setDataLayout(S);
----------------
This assumes that the triple always appears before the data layout. The writer always puts them in the expected order, but just to be safe, I'd move this out of the `while (true)` loop.


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:4122
+  // address spaces to the datalayout.
+  if (!TT.startswith("x86") || DL.contains(AddrSpaces))
+    return DL;
----------------
I would use this code pattern to leverage the Triple parsing logic:
```
Triple::ArchType Arch = Triple(TT).getArch();
if (Arch != llvm::Triple::x86 && Arch != llvm::Triple::x86_64 ...)
```
Otherwise this won't fire for an i686-* triple.


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:4126
+  SmallVector<StringRef, 4> Groups;
+  Regex R("(e-m:[a-z](-p:32:32)?)(-[if]64:.*$)");
+  if (!R.match(DL, &Groups))
----------------
What about -p:64:64 for x64?


================
Comment at: llvm/test/Bitcode/upgrade-datalayout.ll:5-6
+
+target datalayout = "e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnux32"
+
----------------
I think we should use the x64 datalayout with the x64 triple, and then add a separate test for an i686 triple & data layout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67631