[PATCH] D67631: Add AutoUpgrade function to add new address space datalayout string to existing datalayouts.
Reid Kleckner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 18 11:07:01 PDT 2019
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
More information about the llvm-commits
mailing list