Change datalayout compatibility check for X86 to allow datalayouts without the new address spaces.

[akhuang]

Summary

Follow up to https://reviews.llvm.org/D64931 to allow existing IR with the old datalayout to compile.

[lebedev.ri]

I suppose would have alleviated the need for most of that datalayout changes in tests?

[rnk]

In D66843#1648047, @lebedev.ri wrote:

I suppose would have alleviated the need for most of that datalayout changes in tests?

Yes and no. I would've still advocated that we do the update and standardize on the new data layout. This just maintains bitcode backwards compatibility.

@echristo reviewed rL243682 / D11654, so I think he should take a look.

[rnk]

lgtm

Sorry this got lost for two weeks. :(

[akhuang]

Commited in https://reviews.llvm.org/rL371568

[echristo]

Still really not a fan of this way. I think you can change the incoming DL in compatibility cases to add an address space when none exists on x86.

-eric

[rnk]

In D66843#1666367, @echristo wrote:

Still really not a fan of this way. I think you can change the incoming DL in compatibility cases to add an address space when none exists on x86.

-eric

Is that suggestion to handle it in AutoUpgrade.cpp? My rationale for doing it this way was, if a module lacks these address spaces in data layout, it is old bitcode, and it won't use these address spaces for this purpose. But of course, it's unchecked.

[kristina]

This seems to have been causing problems during linking, got hundreds of those:

ld.lld: warning: Linking two modules of different data layouts: '/b/llvm-10/407/lib/libc++.a(cxa_exception.cpp.o at 5739456)' is 'e-m:e-i64:64-f80:128-n8:16:32:64-S128' whereas 'obj/gpr/third_party/protobuf/libprotoc_lib.a(libprotoc_lib.java_message_builder_lite.o at 10565644)' is 'e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128'

If this is only being used for clang-cl (or -fms-compatibility) mode only, is it possible to limit it to just that rather than it affecting pretty much everything?

[rnk]

In D66843#1669728, @kristina wrote:

If this is only being used for clang-cl (or -fms-compatibility) mode only, is it possible to limit it to just that rather than it affecting pretty much everything?

We decided we'd rather not do that. There needs to be some way to upgrade the x86 datalayout. It can't be frozen in stone forever.

Of course, we need to support the use case of linking old bitcode with new bitcode. Doing that probably requires adding an autoupgrade to replace the old data layout with the new one when loading bitcode. We already have x86-specific intrinsic upgrades in llvm/lib/IR/AutoUpgrade.cpp, so it seems reasonable to add this there, even though it is x86 specific, and remove the X86TargetMachine change.

[akhuang]

In D66843#1669803, @rnk wrote:

In D66843#1669728, @kristina wrote:

If this is only being used for clang-cl (or -fms-compatibility) mode only, is it possible to limit it to just that rather than it affecting pretty much everything?

We decided we'd rather not do that. There needs to be some way to upgrade the x86 datalayout. It can't be frozen in stone forever.

Of course, we need to support the use case of linking old bitcode with new bitcode. Doing that probably requires adding an autoupgrade to replace the old data layout with the new one when loading bitcode. We already have x86-specific intrinsic upgrades in llvm/lib/IR/AutoUpgrade.cpp, so it seems reasonable to add this there, even though it is x86 specific, and remove the X86TargetMachine change.

That sounds good; I can work on a patch for that.

[akhuang]

I really do think you should just upgrade old layouts to include the address space.

Maybe I'm misunderstanding--is this different from the AutoUpgrade thing that @rnk suggested?

llvm/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.
-  bool isCompatibleDataLayout(const DataLayout &Candidate) const {
+  virtual 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/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/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) {
+static std::string computeDataLayout(const Triple &TT,
+                                     bool AddressSpaces = true) {
   // 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())) {
+      TLOF(createTLOF(getTargetTriple())),
+      DLNoAddrSpaces(computeDataLayout(TT, /*AddressSpaces=*/false)) {
   // Windows stack unwinder gets confused when execution flow "falls through"
   // after a call to 'noreturn' function.
   // To prevent that, we emit a trap for 'unreachable' IR instructions.
   // (which on X86, happens to be the 'ud2' instruction)
   // 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.
   // The check here for 64-bit windows is a bit icky, but as we're unlikely
   // to ever want to mix 32 and 64-bit windows code in a single module
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));