[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG51e09e1d5aa4: [AMDGPU] Set the default globals address space 
to 1 (authored by arichardson).

Changed prior to commit:
  https://reviews.llvm.org/D84345?vs=289235&id=306693#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

Files:
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGen/target-data.c
  clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
  llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp

Index: llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
===
--- llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
+++ llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
@@ -27,6 +27,10 @@
  "-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) {
@@ -46,6 +50,13 @@
   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");
+  EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-G0", "amdgcn"), "e-p:64:64-G0");
 }
 
 TEST(DataLayoutUpgradeTest, EmptyDataLayout) {
@@ -54,6 +65,10 @@
   "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
Index: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
===
--- llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -342,15 +342,15 @@
 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";
+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-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"
+  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";
 }
 
Index: llvm/lib/IR/AutoUpgrade.cpp
===
--- llvm/lib/IR/AutoUpgrade.cpp
+++ llvm/lib/IR/AutoUpgrade.cpp
@@ -4380,11 +4380,17 @@
 }
 
 std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
-  StringRef 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 Groups;
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -215,7 +215,7 @@
 GV->setAlignment(Align(8));
 Ident = GV;
   }
-  return Ident;
+  return Builder

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.
Herald added a subscriber: dexonsmith.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 289235.
arichardson added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

Files:
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGen/target-data.c
  clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
  llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp

Index: llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
===
--- llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
+++ llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
@@ -27,6 +27,10 @@
  "-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) {
@@ -46,6 +50,13 @@
   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");
+  EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-G0", "amdgcn"), "e-p:64:64-G0");
 }
 
 TEST(DataLayoutUpgradeTest, EmptyDataLayout) {
@@ -54,6 +65,10 @@
   "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
Index: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
===
--- llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -344,15 +344,15 @@
 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";
+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-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"
+  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";
 }
 
Index: llvm/lib/IR/AutoUpgrade.cpp
===
--- llvm/lib/IR/AutoUpgrade.cpp
+++ llvm/lib/IR/AutoUpgrade.cpp
@@ -4398,11 +4398,17 @@
 }
 
 std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
-  StringRef 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 Groups;
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -214,7 +214,7 @@
 GV->setAlignment(Align(8));
 Ident = GV;
   }
-  return Ident;
+  return Builder.CreatePointerCast(Ident, IdentPtr);
 }
 
 Constant *OpenMPIRBuilder::getOrCreateSrcLocStr(StringRef LocStr) {
Index: clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl
=

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 288316.
arichardson added a comment.

- fix failing tests after datalayout change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

Files:
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGen/target-data.c
  clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
  llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp

Index: llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
===
--- llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
+++ llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
@@ -27,6 +27,10 @@
  "-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) {
@@ -46,6 +50,13 @@
   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");
+  EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-G0", "amdgcn"), "e-p:64:64-G0");
 }
 
 TEST(DataLayoutUpgradeTest, EmptyDataLayout) {
@@ -54,6 +65,10 @@
   "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
Index: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
===
--- llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -344,15 +344,15 @@
 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";
+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-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"
+  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";
 }
 
Index: llvm/lib/IR/AutoUpgrade.cpp
===
--- llvm/lib/IR/AutoUpgrade.cpp
+++ llvm/lib/IR/AutoUpgrade.cpp
@@ -4308,11 +4308,17 @@
 }
 
 std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
-  StringRef 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 Groups;
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -214,7 +214,7 @@
 GV->setAlignment(Align(8));
 Ident = GV;
   }
-  return Ident;
+  return Builder.CreatePointerCast(Ident, IdentPtr);
 }
 
 Constant *OpenMPIRBuilder::getOrCreateSrcLocStr(StringRef LocStr) {
Index: clang/test/CodeGenOpenCL/amd

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4297
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+return DL.empty() ? std::string("G1") : (DL + "-G1").str();

dylanmckay wrote:
> dylanmckay wrote:
> > arichardson wrote:
> > > akhuang wrote:
> > > > arichardson wrote:
> > > > > arsenm wrote:
> > > > > > 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
> > > > > 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?
> > > > I don't think so; parsing the datalayout sounds better to me too. 
> > > 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`
> > > 
> > 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`
> 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();
>   }
> ```
We then end up overwriting explicitly specified `G` 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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Dylan McKay via Phabricator via cfe-commits]

dylanmckay added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4297
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+return DL.empty() ? std::string("G1") : (DL + "-G1").str();

dylanmckay wrote:
> arichardson wrote:
> > akhuang wrote:
> > > arichardson wrote:
> > > > arsenm wrote:
> > > > > 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
> > > > 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?
> > > I don't think so; parsing the datalayout sounds better to me too. 
> > 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`
> > 
> 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`
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();
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Dylan McKay via Phabricator via cfe-commits]

dylanmckay added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4297
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+return DL.empty() ? std::string("G1") : (DL + "-G1").str();

arichardson wrote:
> akhuang wrote:
> > arichardson wrote:
> > > arsenm wrote:
> > > > 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
> > > 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?
> > I don't think so; parsing the datalayout sounds better to me too. 
> 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`
> 
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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson marked an inline comment as done.
arichardson added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4297
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+return DL.empty() ? std::string("G1") : (DL + "-G1").str();

akhuang wrote:
> arichardson wrote:
> > arsenm wrote:
> > > 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
> > 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?
> I don't think so; parsing the datalayout sounds better to me too. 
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`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Amy Huang via Phabricator via cfe-commits]

akhuang added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4297
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+return DL.empty() ? std::string("G1") : (DL + "-G1").str();

arichardson wrote:
> arsenm wrote:
> > 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
> 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?
I don't think so; parsing the datalayout sounds better to me too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson marked an inline comment as done.
arichardson added a subscriber: akhuang.
arichardson added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4297
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+return DL.empty() ? std::string("G1") : (DL + "-G1").str();

arsenm wrote:
> 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
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4297
+  // address space of 1.
+  if (T.isAMDGPU() && !DL.contains("-G") && !DL.startswith("G")) {
+return DL.empty() ? std::string("G1") : (DL + "-G1").str();

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84345: [AMDGPU] Set the default globals address space to 1

[Alexander Richardson via Phabricator via cfe-commits]

arichardson created this revision.
arichardson added a reviewer: arsenm.
Herald added subscribers: llvm-commits, cfe-commits, kerbowa, hiraditya, t-tye, 
tpr, dstuttard, yaxunl, nhaehnle, wdng, jvesely, kzhuravl.
Herald added projects: clang, LLVM.

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 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84345

Files:
  clang/lib/Basic/Targets/AMDGPU.cpp
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
  llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp

Index: llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
===
--- llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
+++ llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp
@@ -27,6 +27,10 @@
  "-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) {
@@ -46,6 +50,12 @@
   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) {
@@ -54,6 +64,10 @@
   "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
Index: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
===
--- llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -345,15 +345,15 @@
 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";
+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-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"
+  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";
 }
 
Index: llvm/lib/IR/AutoUpgrade.cpp
===
--- llvm/lib/IR/AutoUpgrade.cpp
+++ llvm/lib/IR/AutoUpgrade.cpp
@@ -4291,11 +4291,17 @@
 }
 
 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 Groups;
Index: clang/lib/Basic/Targets/AMDGPU.cpp
===
--- clang/lib/Basic/Targets/AMDGPU.cpp
+++ clang/lib/Basic/Targets/AMDGPU.cpp
@@ -31,12 +31,12 @@
 
 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:204