[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/IR/AutoUpgrade.cpp:4126
+  SmallVector<StringRef, 4> Groups;
+  Regex R("(e-m:[a-z](-p:32:32)?)(-[if]64:.*$)");
+  if (!R.match(DL, &Groups))
----------------
akhuang wrote:
> rnk wrote:
> > What about -p:64:64 for x64?
> I was basing the pattern off of the computeDataLayout function in X86TargetMachine.cpp, which doesn't seem to add -p:64:64. There are a bunch of tests that have -p:64:64:64 in the DL, but I think those wouldn't pass the DL assert anyway if they were in a test case where it matters?
Oh, I see, this is only for x32-like targets.


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:4132-4134
+  SmallString<1024> Buf;
+  StringRef Res = (Groups[1] + AddrSpaces + Groups[3]).toStringRef(Buf);
+  return Res;
----------------
This looks like use-after-return. I think you can do the concatenation and call `.str()` on the result to get a std::string, which is safe to return.


================
Comment at: llvm/test/Bitcode/upgrade-datalayout2.ll:6-7
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
You know, it occurs to me that these might be better as unit tests. The prototype of llvm::UpgradeDataLayout takes and returns strings, so it would be really easy to do these in gtest. I think it's actually worth taking the time to do that.

We might also want to write some negative tests for cases that we don't want to upgrade.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67631