[AMDGPU] Set the default globals address space to 1

[arichardson]

Summary

This will ensure that passes that add new global variables will create them
in address space 1 once the passes have been updated to no longer default
to the implicit address space zero.
This also changes AutoUpgrade.cpp to add -G1 to the DataLayout if it wasn't
already to present to ensure bitcode backwards compatibility.

Depends on D70947

[arsenm]

llvm/lib/IR/AutoUpgrade.cpp

4297

I would expect datalayout upgrades to work by parsing the old string, and checking the field values inside. I guess directly checking the string isn't a new problem here

[arichardson]

llvm/lib/IR/AutoUpgrade.cpp

4297

I agree that would be less error prone. I wonder if there are cases where the old string may fail to parse so you have to do the textual upgrade first. I'm happy to make this change.

@akhuang is there a reason you used string parsing in D67631? Any objections to changing the code to parse the datalayout and add missing attributes?

[akhuang]

llvm/lib/IR/AutoUpgrade.cpp

4297

I don't think so; parsing the datalayout sounds better to me too.

[arichardson]

llvm/lib/IR/AutoUpgrade.cpp

4297

I just looked into parsing the DataLayout instead. Unfortunately the resulting code is more complicated since there are no setters in DataLayout and no way to create a normalized representation.
There's also no way to differentiate between no -G passed and -G0 so something like e-p:64:64-G0 will be converted to e-p:64:64-G0-G1

[dylanmckay]

llvm/lib/IR/AutoUpgrade.cpp

4297

I suspect it would be possible to use the existing DataLayout(StringRef) constructor on the string, then call getDefaultGlobalsAddressSpace() on it, explicitly ignoring modifying the datalayout for the special case of an explicit -G0.

For example,

cpp
  DataLayout ParsedDL = DataLayout(DL);
  if (T.isAMDGPU() && !DL.contains("-G0") &&ParsedDL.getDefaultGlobalsAddressSpace() != 1) {
    return DL.empty() ? std::string("G1") : (DL + "-G1").str();
  }

As I understand it, this would cover the fact that we cannot distinguish between an explicit default globals space of zero, and a datalayout without a default globals space (also DL::getDefaultGlobalsAddressSpace() == 0) by explicitly excluding the special case -G0

[dylanmckay]

llvm/lib/IR/AutoUpgrade.cpp

4297

To be completely correct it should not assume that the global address space is not at the very start of the data layout as my initial snippet did. I've removed the - prefix from the contains check

DataLayout ParsedDL = DataLayout(DL);
if (T.isAMDGPU() && !DL.contains("G0") &&ParsedDL.getDefaultGlobalsAddressSpace() != 1) {
  return DL.empty() ? std::string("G1") : (DL + "-G1").str();
}

[arichardson]

llvm/lib/IR/AutoUpgrade.cpp

4297

We then end up overwriting explicitly specified G<N> flags. Maybe some tests would like to check that overriding the globals address space works? Also I believe this can result in an invalid datalayout: "...-G2" will be converted to "...-G2-G1".

Not sure what the correct approach is @arsenm ?

[arichardson]

• fix failing tests after datalayout change

[arichardson]

clang-format

[arichardson]

ping?

[arichardson]

ping @arsenm . I'd really like to land D70947 so that I can upstream further changes and that review is blocked on this one.

clang/lib/Basic/Targets/AMDGPU.cpp

 namespace clang {
 namespace targets {
 
 // If you edit the description strings, make sure you update
 // getPointerWidthV().
 
 static const char *const DataLayoutStringR600 =
     "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
-    "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5";
+    "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1";
 
 static const char *const DataLayoutStringAMDGCN =
     "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32"
     "-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
-    "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+    "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1"
     "-ni:7";
 
 const LangASMap AMDGPUTargetInfo::AMDGPUDefIsGenMap = {
     Generic,  // Default
     Global,   // opencl_global
     Local,    // opencl_local
     Constant, // opencl_constant
     Private,  // opencl_private

llvm/lib/IR/AutoUpgrade.cpp

MDNode *llvm::upgradeInstructionLoopAttachment(MDNode &N) {
   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";
+  Triple T(TT);
+  // For AMDGPU we uprgrade older DataLayouts to include the default globals
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+    return DL.empty() ? std::string("G1") : (DL + "-G1").str();
+  }
 
+  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.
-  if (!Triple(TT).isX86() || DL.contains(AddrSpaces))
+  if (!T.isX86() || DL.contains(AddrSpaces))
     return std::string(DL);
 
   SmallVector<StringRef, 4> Groups;
   Regex R("(e-m:[a-z](-p:32:32)?)(-[if]64:.*$)");
   if (!R.match(DL, &Groups))
     return std::string(DL);
 
   SmallString<1024> Buf;

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

 static MachineSchedRegistry
 GCNILPSchedRegistry("gcn-ilp",
   "Run GCN iterative scheduler for ILP scheduling (experimental)",
   createIterativeILPMachineScheduler);
 
 static StringRef computeDataLayout(const Triple &TT) {
   if (TT.getArch() == Triple::r600) {
     // 32-bit pointers.
-      return "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
-             "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5";
+           "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1";
   }
 
   // 32-bit private, local, and region pointers. 64-bit global, constant and
   // flat, non-integral buffer fat pointers.
-    return "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32"
          "-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
-         "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+         "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1"
          "-ni:7";
 }
 
 LLVM_READNONE
 static StringRef getGPUOrDefault(const Triple &TT, StringRef GPU) {
   if (!GPU.empty())
     return GPU;
 

llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp

TEST(DataLayoutUpgradeTest, ValidDataLayoutUpgrade) {
   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");
+
+  // Check that AMDGPU targets add -G1 if it's not present.
+  EXPECT_EQ(UpgradeDataLayoutString("e-p:32:32", "r600"), "e-p:32:32-G1");
+  EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64", "amdgcn"), "e-p:64:64-G1");
 }
 
 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");
+
+  // Check that AMDGPU targets don't add -G1 if there is already a -G flag.
+  EXPECT_EQ(UpgradeDataLayoutString("e-p:32:32-G2", "r600"), "e-p:32:32-G2");
+  EXPECT_EQ(UpgradeDataLayoutString("G2", "r600"), "G2");
+  EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-G2", "amdgcn"), "e-p:64:64-G2");
+  EXPECT_EQ(UpgradeDataLayoutString("G2-e-p:64:64", "amdgcn"), "G2-e-p:64:64");
 }
 
 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");
+
+  // Check that AMDGPU targets add G1 if it's not present.
+  EXPECT_EQ(UpgradeDataLayoutString("", "r600"), "G1");
+  EXPECT_EQ(UpgradeDataLayoutString("", "amdgcn"), "G1");
 }
 
 } // end namespace