Add AutoUpgrade function to add new address space datalayout string to existing datalayouts.

[akhuang]

Summary

Add function to AutoUpgrade to change the datalayout of old X86 datalayout strings.
This adds "-p270:32:32-p271:32:32-p272:64:64" to X86 datalayouts that are otherwise valid
and don't already contain it.

This also removes the compatibility changes in https://reviews.llvm.org/D66843.
Datalayout change in https://reviews.llvm.org/D64931.

[echristo]

Thanks! Idea sounds great, I'm out on vacation so I'll ask rnk to review :)

[akhuang]

Add test cases

[rnk]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3583 ↗

(On Diff #220384)

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.

llvm/lib/IR/AutoUpgrade.cpp

4122 ↗

(On Diff #220384)

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.

4126 ↗

(On Diff #220384)

What about -p:64:64 for x64?

llvm/test/Bitcode/upgrade-datalayout.ll

5–6 ↗

(On Diff #220384)

I think we should use the x64 datalayout with the x64 triple, and then add a separate test for an i686 triple & data layout.

[akhuang]

llvm/lib/IR/AutoUpgrade.cpp

4126 ↗

(On Diff #220384)

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?

[akhuang]

• Address comments

[rnk]

llvm/lib/IR/AutoUpgrade.cpp

4126 ↗

(On Diff #220384)

Oh, I see, this is only for x32-like targets.

4132–4134 ↗

(On Diff #220591)

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.

llvm/test/Bitcode/upgrade-datalayout2.ll

6–7 ↗

(On Diff #220591)

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.

[akhuang]

• Add unittests
• Change return type to std::string

[akhuang]

llvm/test/Bitcode/upgrade-datalayout2.ll

6–7 ↗

(On Diff #220591)

I added a unittest file with some upgrade tests and some negative tests.

[rnk]

lgtm

[akhuang]

clang format

[arichardson]

llvm/trunk/include/llvm/Target/TargetMachine.h

160

Why was virtual removed here? We override this in the MIPS backend for our CHERI fork.

If the intention is to not allow targets to override this, then the documentation also needs to be adjusted.
However, I doubt avoiding the virtual function call gives any measurable performance speedup.

[rnk]

llvm/trunk/include/llvm/Target/TargetMachine.h

160

I dug through the history trying to figure out why it was made virtual, and wasn't able to find any strong reason for it. Obviously, no in tree targets use it. If you want to add it back, I'd approve such a patch.

Also, I seem to recall that overriding this wasn't actually enough for our use cases anyway, so it didn't seem that useful.

[arichardson]

llvm/trunk/include/llvm/Target/TargetMachine.h

160

Looking through our history I was not able to see why we added the override. It was added during a merge from upstream with no explanation. I just removed the override and all tests still pass so it no longer appears necessary. Sorry for the noise.

Although I guess the This hook provides a target specific check on the validity of this DataLayout. comment should probably be deleted.

llvm/trunk/include/llvm/IR/AutoUpgrade.h

namespace llvm {
   /// Check whether a string looks like an old loop attachment tag.
   inline bool mayBeOldLoopAttachmentTag(StringRef Name) {
     return Name.startswith("llvm.vectorizer.");
   }
 
   /// Upgrade the loop attachment metadata node.
   MDNode *upgradeInstructionLoopAttachment(MDNode &N);
 
+  /// Upgrade the datalayout string by adding a section for address space
+  /// pointers.
+  std::string UpgradeDataLayoutString(StringRef DL, StringRef Triple);
+
 } // End llvm namespace
 
 #endif

llvm/trunk/include/llvm/Target/TargetMachine.h

public:
   /// Create a DataLayout.
   const DataLayout createDataLayout() const { return DL; }
 
   /// Test if a DataLayout if compatible with the CodeGen for this target.
   ///
   /// The LLVM Module owns a DataLayout that is used for the target independent
   /// optimizations and code generation. This hook provides a target specific
   /// check on the validity of this DataLayout.
-  virtual bool isCompatibleDataLayout(const DataLayout &Candidate) const {
+  bool isCompatibleDataLayout(const DataLayout &Candidate) const {
     return DL == Candidate;
   }
 
   /// Get the pointer size for this target.
   ///
   /// This is the only time the DataLayout in the TargetMachine is used.
   unsigned getPointerSize(unsigned AS) const {
     return DL.getPointerSize(AS);

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

while (true) {
     case bitc::MODULE_CODE_SOURCE_FILENAME:
       SmallString<128> ValueName;
       if (convertToString(Record, 0, ValueName))
         return error("Invalid record");
       TheModule->setSourceFileName(ValueName);
       break;
     }
     Record.clear();
+
+    // Upgrade data layout string.
+    std::string DL = llvm::UpgradeDataLayoutString(
+        TheModule->getDataLayoutStr(), TheModule->getTargetTriple());
+    TheModule->setDataLayout(DL);
   }
 }
 
 Error BitcodeReader::parseBitcodeInto(Module *M, bool ShouldLazyLoadMetadata,
                                       bool IsImporting) {
   TheModule = M;
   MDLoader = MetadataLoader(Stream, *M, ValueList, IsImporting,
                             [&](unsigned ID) { return getTypeByID(ID); });

llvm/trunk/lib/IR/AutoUpgrade.cpp

MDNode *llvm::upgradeInstructionLoopAttachment(MDNode &N) {
 
   SmallVector<Metadata *, 8> Ops;
   Ops.reserve(T->getNumOperands());
   for (Metadata *MD : T->operands())
     Ops.push_back(upgradeLoopArgument(MD));
 
   return MDTuple::get(T->getContext(), Ops);
 }
+
+std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
+  std::string AddrSpaces = "-p270:32:32-p271:32:32-p272:64:64";
+
+  // If X86, and the datalayout matches the expected format, add pointer size
+  // address spaces to the datalayout.
+  Triple::ArchType Arch = Triple(TT).getArch();
+  if ((Arch != llvm::Triple::x86 && Arch != llvm::Triple::x86_64) ||
+      DL.contains(AddrSpaces))
+    return DL;
+
+  SmallVector<StringRef, 4> Groups;
+  Regex R("(e-m:[a-z](-p:32:32)?)(-[if]64:.*$)");
+  if (!R.match(DL, &Groups))
+    return DL;
+
+  SmallString<1024> Buf;
+  std::string Res = (Groups[1] + AddrSpaces + Groups[3]).toStringRef(Buf).str();
+  return Res;
+}

llvm/trunk/lib/Target/X86/X86TargetMachine.h

 
 class StringRef;
 class X86Subtarget;
 class X86RegisterBankInfo;
 
 class X86TargetMachine final : public LLVMTargetMachine {
   std::unique_ptr<TargetLoweringObjectFile> TLOF;
   mutable StringMap<std::unique_ptr<X86Subtarget>> SubtargetMap;
-  const DataLayout DLNoAddrSpaces;
 
 public:
   X86TargetMachine(const Target &T, const Triple &TT, StringRef CPU,
                    StringRef FS, const TargetOptions &Options,
                    Optional<Reloc::Model> RM, Optional<CodeModel::Model> CM,
                    CodeGenOpt::Level OL, bool JIT);
   ~X86TargetMachine() override;
 
   const X86Subtarget *getSubtargetImpl(const Function &F) const override;
   // DO NOT IMPLEMENT: There is no such thing as a valid default subtarget,
   // subtargets are per-function entities based on the target-specific
   // attributes of each function.
   const X86Subtarget *getSubtargetImpl() const = delete;
 
   TargetTransformInfo getTargetTransformInfo(const Function &F) override;
 
   // Set up the pass pipeline.
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
-
-  bool isCompatibleDataLayout(const DataLayout &Candidate) const override;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_X86_X86TARGETMACHINE_H

llvm/trunk/lib/Target/X86/X86TargetMachine.cpp

if (TT.isOSFuchsia())
     return std::make_unique<X86FuchsiaTargetObjectFile>();
   if (TT.isOSBinFormatELF())
     return std::make_unique<X86ELFTargetObjectFile>();
   if (TT.isOSBinFormatCOFF())
     return std::make_unique<TargetLoweringObjectFileCOFF>();
   llvm_unreachable("unknown subtarget type");
 }
 
-static std::string computeDataLayout(const Triple &TT,
-                                     bool AddressSpaces = true) {
+static std::string computeDataLayout(const Triple &TT) {
   // X86 is little endian
   std::string Ret = "e";
 
   Ret += DataLayout::getManglingComponent(TT);
   // X86 and x32 have 32 bit pointers.
   if ((TT.isArch64Bit() &&
        (TT.getEnvironment() == Triple::GNUX32 || TT.isOSNaCl())) ||
       !TT.isArch64Bit())
     Ret += "-p:32:32";
 
   // Address spaces for 32 bit signed, 32 bit unsigned, and 64 bit pointers.
-  if (AddressSpaces)
-    Ret += "-p270:32:32-p271:32:32-p272:64:64";
 
   // Some ABIs align 64 bit integers and doubles to 64 bits, others to 32.
   if (TT.isArch64Bit() || TT.isOSWindows() || TT.isOSNaCl())
     Ret += "-i64:64";
   else if (TT.isOSIAMCU())
     Ret += "-i64:32-f64:32";
   else
     Ret += "-f64:32:64";
X86TargetMachine::X86TargetMachine(const Target &T, const Triple &TT,
                                    Optional<Reloc::Model> RM,
                                    Optional<CodeModel::Model> CM,
                                    CodeGenOpt::Level OL, bool JIT)
     : LLVMTargetMachine(
           T, computeDataLayout(TT), TT, CPU, FS, Options,
           getEffectiveRelocModel(TT, JIT, RM),
           getEffectiveX86CodeModel(CM, JIT, TT.getArch() == Triple::x86_64),
           OL),
-      TLOF(createTLOF(getTargetTriple())),
-      DLNoAddrSpaces(computeDataLayout(TT, /*AddressSpaces=*/false)) {
+      TLOF(createTLOF(getTargetTriple())) {
   // On PS4, the "return address" of a 'noreturn' call must still be within
   // the calling function, and TrapUnreachable is an easy way to get that.
   if (TT.isPS4() || TT.isOSBinFormatMachO()) {
     this->Options.TrapUnreachable = true;
     this->Options.NoTrapAfterNoreturn = TT.isOSBinFormatMachO();
   }
 
   // Outlining is available for x86-64.
if (!I) {
     I = std::make_unique<X86Subtarget>(TargetTriple, CPU, FS, *this,
                                         Options.StackAlignmentOverride,
                                         PreferVectorWidthOverride,
                                         RequiredVectorWidth);
   }
   return I.get();
 }
 
-bool X86TargetMachine::isCompatibleDataLayout(
-    const DataLayout &Candidate) const {
-  // Maintain compatibility with datalayouts that don't have address space
-  // pointer sizes.
-  return DL == Candidate || DLNoAddrSpaces == Candidate;
-}
-
 //===----------------------------------------------------------------------===//
 // Command line options for x86
 //===----------------------------------------------------------------------===//
 static cl::opt<bool>
 UseVZeroUpper("x86-use-vzeroupper", cl::Hidden,
   cl::desc("Minimize AVX to SSE transition penalty"),
   cl::init(true));
 

llvm/trunk/test/Bitcode/upgrade-datalayout.ll

+; Test to make sure datalayout is automatically upgraded.
+;
+; RUN: llvm-as %s -o - | llvm-dis - | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+

llvm/trunk/test/Bitcode/upgrade-datalayout2.ll

+; Test to make sure datalayout is not automatically upgraded if it does not
+; match a possible x86 datalayout.
+;
+; RUN: llvm-as %s -o - | llvm-dis - | FileCheck %s
+
+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"
+
+; CHECK: 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"
+

llvm/trunk/test/Bitcode/upgrade-datalayout3.ll

+; Test to make sure datalayout is automatically upgraded.
+;
+; RUN: llvm-as %s -o - | llvm-dis - | FileCheck %s
+
+target datalayout = "e-m:w-p:32:32-i64:64-f80:32-n8:16:32-S32"
+target triple = "i686-pc-windows-msvc"
+
+; CHECK: target datalayout = "e-m:w-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:32-n8:16:32-S32"

llvm/trunk/unittests/Bitcode/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   AsmParser
   BitReader
   BitWriter
   Core
   Support
   )
 
 add_llvm_unittest(BitcodeTests
   BitReaderTest.cpp
+  DataLayoutUpgradeTest.cpp
   )

llvm/trunk/unittests/Bitcode/DataLayoutUpgradeTest.cpp

+//===- DataLayoutUpgradeTest.cpp - Tests for DataLayout upgrades ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/IR/AutoUpgrade.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(DataLayoutUpgradeTest, ValidDataLayoutUpgrade) {
+  std::string DL1 =
+      UpgradeDataLayoutString("e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128",
+                              "x86_64-unknown-linux-gnu");
+  std::string DL2 = UpgradeDataLayoutString(
+      "e-m:w-p:32:32-i64:64-f80:32-n8:16:32-S32", "i686-pc-windows-msvc");
+  std::string DL3 = UpgradeDataLayoutString("e-m:o-i64:64-i128:128-n32:64-S128",
+                                            "x86_64-apple-macosx");
+  EXPECT_EQ(DL1, "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64"
+                 "-f80:128-n8:16:32:64-S128");
+  EXPECT_EQ(DL2, "e-m:w-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64"
+                 "-f80:32-n8:16:32-S32");
+  EXPECT_EQ(DL3, "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128"
+                 "-n32:64-S128");
+}
+
+TEST(DataLayoutUpgradeTest, NoDataLayoutUpgrade) {
+  std::string DL1 = UpgradeDataLayoutString(
+      "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",
+      "x86_64-unknown-linux-gnu");
+  std::string DL2 = UpgradeDataLayoutString("e-p:32:32", "i686-apple-darwin9");
+  std::string DL3 = UpgradeDataLayoutString("e-m:e-i64:64-n32:64",
+                                            "powerpc64le-unknown-linux-gnu");
+  std::string DL4 =
+      UpgradeDataLayoutString("e-m:o-i64:64-i128:128-n32:64-S128", "aarch64--");
+  EXPECT_EQ(DL1, "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");
+  EXPECT_EQ(DL2, "e-p:32:32");
+  EXPECT_EQ(DL3, "e-m:e-i64:64-n32:64");
+  EXPECT_EQ(DL4, "e-m:o-i64:64-i128:128-n32:64-S128");
+}
+
+TEST(DataLayoutUpgradeTest, EmptyDataLayout) {
+  std::string DL1 = UpgradeDataLayoutString("", "x86_64-unknown-linux-gnu");
+  std::string DL2 = UpgradeDataLayoutString(
+      "e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128", "");
+  EXPECT_EQ(DL1, "");
+  EXPECT_EQ(DL2, "e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128");
+}
+
+} // end namespace