On 07/23/2013 11:36 PM, Tanya Lattner wrote:
However, I actually should have looked at this closer as it actually doesn't map
to what I want it to. I want it to be the following:

1, // opencl_global
3, // opencl_local
2, // opencl_constant

and when there is no address space then it maps to nothing.

So, I don't think your patch is going to work unless the order is changed in the
enum. Because this is not clearly defined in the spec and is implementation
specific and TARGET specific..  then changing that enum is probably not going to
be the right approach either.

I see the point but still we need to preserve the source language difference with the mangling.

So, I'm going back to my original statement to keep it to be Target specific.
For your library, are these functions actually implemented differently? Wouldn't
they be exactly the same when there is no address space? In our implementation
we have an address space map defined for X86 and then  the names get mangled
"correctly" for all targets. But, all the functionality is the same since the
address spaces don't impact codegen for X86.

I'd argue that the fact that address spaces do not impact codegen for X86 is merely incidental: the issue goes beyond X86 and impacts all targets -- including future ones. By choosing to use a fake address space map (instead of the one fitting the target description), we introduce in the IR a potential for breaking future implementations, even though the current X86 target is not affected. Moreover, by introducing the fake address map we would violate the semantics of the LLVM IR addrspace modifier.

Even binary compatibility with libraries already distributed can be easily 
achieved.

This patch (see attachment) aims at preserving the mangling that were generated by using the target address space map for those targets that override it, while introducing in the mangling the distinction of opencl/cuda address spaces for those targets that do not have a non trivial target address space map.

By the way, looking beyond the scope of the current issue of mangling, it would be IMO interesting to start a public discussion on the mailing list about a way to represent logical address space information different from target-specific address space (the case for OpenCL and CUDA) in order to allow the implementation of custom language specific analysis and/or optimization. As a temporary solution, if one needed the logical address space information in the IR too for specific purpose (like OpenCL specific optimization), can still override the address space map of the target.

Thanks again.

Regards,

-Michele

diff --git a/include/clang/AST/ASTContext.h b/include/clang/AST/ASTContext.h
index a686f33..cfacfdf 100644
--- a/include/clang/AST/ASTContext.h
+++ b/include/clang/AST/ASTContext.h
@@ -378,8 +378,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
   OwningPtr<CXXABI> ABI;
   CXXABI *createCXXABI(const TargetInfo &T);
 
-  /// \brief The logical -> physical address space map.
-  const LangAS::Map *AddrSpaceMap;
+  /// \brief The internal -> physical address space map.
+  const LangAS::Map *PhysicalAddrSpaceMap;
+
+  /// \brief The internal -> logical address space map.
+  const LangAS::Map *LogicalAddrSpaceMap;
 
   friend class ASTDeclReader;
   friend class ASTReader;
@@ -1883,19 +1886,21 @@ public:
   QualType getFloatingTypeOfSizeWithinDomain(QualType typeSize,
                                              QualType typeDomain) const;
 
-  unsigned getTargetAddressSpace(QualType T) const {
-    return getTargetAddressSpace(T.getQualifiers());
+  unsigned getTargetAddressSpace(QualType T, bool Logical = false) const {
+    return getTargetAddressSpace(T.getQualifiers(), Logical);
   }
 
-  unsigned getTargetAddressSpace(Qualifiers Q) const {
-    return getTargetAddressSpace(Q.getAddressSpace());
+  unsigned getTargetAddressSpace(Qualifiers Q, bool Logical = false) const {
+    return getTargetAddressSpace(Q.getAddressSpace(), Logical);
   }
 
-  unsigned getTargetAddressSpace(unsigned AS) const {
+  unsigned getTargetAddressSpace(unsigned AS, bool Logical = false) const {
     if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
       return AS;
+    else if (Logical)
+      return (*LogicalAddrSpaceMap)[AS - LangAS::Offset];
     else
-      return (*AddrSpaceMap)[AS - LangAS::Offset];
+      return (*PhysicalAddrSpaceMap)[AS - LangAS::Offset];
   }
 
 private:
diff --git a/include/clang/Basic/AddressSpaces.h b/include/clang/Basic/AddressSpaces.h
index 4b1cea5..632e418 100644
--- a/include/clang/Basic/AddressSpaces.h
+++ b/include/clang/Basic/AddressSpaces.h
@@ -28,8 +28,8 @@ enum ID {
   Offset = 0xFFFF00,
 
   opencl_global = Offset,
-  opencl_local,
   opencl_constant,
+  opencl_local,
 
   cuda_device,
   cuda_constant,
diff --git a/include/clang/Basic/TargetInfo.h b/include/clang/Basic/TargetInfo.h
index 6002836..4399176 100644
--- a/include/clang/Basic/TargetInfo.h
+++ b/include/clang/Basic/TargetInfo.h
@@ -76,7 +76,8 @@ protected:
     *LongDoubleFormat;
   unsigned char RegParmMax, SSERegParmMax;
   TargetCXXABI TheCXXABI;
-  const LangAS::Map *AddrSpaceMap;
+  const LangAS::Map *PhysicalAddrSpaceMap;
+  const LangAS::Map *LogicalAddrSpaceMap;
 
   mutable StringRef PlatformName;
   mutable VersionTuple PlatformMinVersion;
@@ -727,8 +728,12 @@ public:
     return 0;
   }
 
-  const LangAS::Map &getAddressSpaceMap() const {
-    return *AddrSpaceMap;
+  const LangAS::Map &getPhysicalAddressSpaceMap() const {
+    return *PhysicalAddrSpaceMap;
+  }
+
+  const LangAS::Map &getLogicalAddressSpaceMap() const {
+    return *LogicalAddrSpaceMap;
   }
 
   /// \brief Retrieve the name of the platform as it is used in the
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
index 8a252f8..a4162e6 100644
--- a/lib/AST/ASTContext.cpp
+++ b/lib/AST/ASTContext.cpp
@@ -669,7 +669,8 @@ CXXABI *ASTContext::createCXXABI(const TargetInfo &T) {
 }
 
 static const LangAS::Map *getAddressSpaceMap(const TargetInfo &T,
-                                             const LangOptions &LOpts) {
+                                             const LangOptions &LOpts,
+                                             bool Logical) {
   if (LOpts.FakeAddressSpaceMap) {
     // The fake address space map must have a distinct entry for each
     // language-specific address space.
@@ -682,8 +683,10 @@ static const LangAS::Map *getAddressSpaceMap(const TargetInfo &T,
       6  // cuda_shared
     };
     return &FakeAddrSpaceMap;
+  } else if (Logical) {
+    return &T.getLogicalAddressSpaceMap();
   } else {
-    return &T.getAddressSpaceMap();
+    return &T.getPhysicalAddressSpaceMap();
   }
 }
 
@@ -710,7 +713,8 @@ ASTContext::ASTContext(LangOptions& LOpts, SourceManager &SM,
     NullTypeSourceInfo(QualType()), 
     FirstLocalImport(), LastLocalImport(),
     SourceMgr(SM), LangOpts(LOpts), 
-    AddrSpaceMap(0), Target(t), PrintingPolicy(LOpts),
+    PhysicalAddrSpaceMap(0), LogicalAddrSpaceMap(0), 
+    Target(t), PrintingPolicy(LOpts),
     Idents(idents), Selectors(sels),
     BuiltinInfo(builtins),
     DeclarationNames(*this),
@@ -885,7 +889,8 @@ void ASTContext::InitBuiltinTypes(const TargetInfo &Target) {
   this->Target = &Target;
   
   ABI.reset(createCXXABI(Target));
-  AddrSpaceMap = getAddressSpaceMap(Target, LangOpts);
+  PhysicalAddrSpaceMap = getAddressSpaceMap(Target, LangOpts, false);
+  LogicalAddrSpaceMap = getAddressSpaceMap(Target, LangOpts, true);
   
   // C99 6.2.5p19.
   InitBuiltinType(VoidTy,              BuiltinType::Void);
diff --git a/lib/AST/ItaniumMangle.cpp b/lib/AST/ItaniumMangle.cpp
index 76a8bf4..2e955a8 100644
--- a/lib/AST/ItaniumMangle.cpp
+++ b/lib/AST/ItaniumMangle.cpp
@@ -1750,9 +1750,12 @@ void CXXNameMangler::mangleQualifiers(Qualifiers Quals) {
     // 
     // where <address-space-number> is a source name consisting of 'AS' 
     // followed by the address space <number>.
+
+    unsigned AS = Quals.getAddressSpace();
+
     SmallString<64> ASString;
     ASString = "AS" + llvm::utostr_32(
-        Context.getASTContext().getTargetAddressSpace(Quals.getAddressSpace()));
+        Context.getASTContext().getTargetAddressSpace(AS, /* Logical */true));
     Out << 'U' << ASString.size() << ASString;
   }
   
diff --git a/lib/Basic/TargetInfo.cpp b/lib/Basic/TargetInfo.cpp
index f8c10d1..eadf3ef 100644
--- a/lib/Basic/TargetInfo.cpp
+++ b/lib/Basic/TargetInfo.cpp
@@ -21,7 +21,15 @@
 #include <cstdlib>
 using namespace clang;
 
-static const LangAS::Map DefaultAddrSpaceMap = { 0 };
+static const LangAS::Map DefaultPhysicalAddrSpaceMap = { 0 };
+static const LangAS::Map DefaultLogicalAddrSpaceMap = {
+  1, // opencl_global
+  2, // opencl_constant
+  3, // opencl_local
+  4, // cuda_device
+  5, // cuda_constant
+  6  // cuda_shared
+};
 
 // TargetInfo Constructor.
 TargetInfo::TargetInfo(const llvm::Triple &T) : TargetOpts(), Triple(T) {
@@ -87,7 +95,9 @@ TargetInfo::TargetInfo(const llvm::Triple &T) : TargetOpts(), Triple(T) {
   TheCXXABI.set(TargetCXXABI::GenericItanium);
 
   // Default to an empty address space map.
-  AddrSpaceMap = &DefaultAddrSpaceMap;
+  PhysicalAddrSpaceMap = &DefaultPhysicalAddrSpaceMap;
+
+  LogicalAddrSpaceMap = &DefaultLogicalAddrSpaceMap;
 
   // Default to an unknown platform name.
   PlatformName = "unknown";
diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp
index c236924..093010b 100644
--- a/lib/Basic/Targets.cpp
+++ b/lib/Basic/Targets.cpp
@@ -1232,8 +1232,8 @@ public:
 namespace {
   static const unsigned NVPTXAddrSpaceMap[] = {
     1,    // opencl_global
-    3,    // opencl_local
     4,    // opencl_constant
+    3,    // opencl_local
     1,    // cuda_device
     4,    // cuda_constant
     3,    // cuda_shared
@@ -1247,7 +1247,7 @@ namespace {
       BigEndian = false;
       TLSSupported = false;
       LongWidth = LongAlign = 64;
-      AddrSpaceMap = &NVPTXAddrSpaceMap;
+      PhysicalAddrSpaceMap = LogicalAddrSpaceMap = &NVPTXAddrSpaceMap;
       // Define available target features
       // These must be defined in sorted order!
       NoAsmVariants = true;
@@ -1369,8 +1369,8 @@ namespace {
 
 static const unsigned R600AddrSpaceMap[] = {
   1,    // opencl_global
-  3,    // opencl_local
   2,    // opencl_constant
+  3,    // opencl_local
   1,    // cuda_device
   2,    // cuda_constant
   3     // cuda_shared
@@ -1419,7 +1419,7 @@ public:
   R600TargetInfo(const llvm::Triple &Triple)
       : TargetInfo(Triple), GPU(GK_R600) {
     DescriptionString = DescriptionStringR600;
-    AddrSpaceMap = &R600AddrSpaceMap;
+    PhysicalAddrSpaceMap = LogicalAddrSpaceMap = &R600AddrSpaceMap;
   }
 
   virtual const char * getClobbers() const {
@@ -4534,8 +4534,8 @@ namespace {
 
   static const unsigned TCEOpenCLAddrSpaceMap[] = {
       3, // opencl_global
-      4, // opencl_local
       5, // opencl_constant
+      4, // opencl_local
       0, // cuda_device
       0, // cuda_constant
       0  // cuda_shared
@@ -4570,7 +4570,7 @@ namespace {
                           "i16:16:32-i32:32:32-i64:32:32-"
                           "f32:32:32-f64:32:32-v64:32:32-"
                           "v128:32:32-a0:0:32-n32";
-      AddrSpaceMap = &TCEOpenCLAddrSpaceMap;
+      PhysicalAddrSpaceMap = LogicalAddrSpaceMap = &TCEOpenCLAddrSpaceMap;
     }
 
     virtual void getTargetDefines(const LangOptions &Opts,
@@ -5127,8 +5127,8 @@ void PNaClTargetInfo::getGCCRegAliases(const GCCRegAlias *&Aliases,
 namespace {
   static const unsigned SPIRAddrSpaceMap[] = {
     1,    // opencl_global
-    3,    // opencl_local
     2,    // opencl_constant
+    3,    // opencl_local
     0,    // cuda_device
     0,    // cuda_constant
     0     // cuda_shared
@@ -5146,7 +5146,7 @@ namespace {
       BigEndian = false;
       TLSSupported = false;
       LongWidth = LongAlign = 64;
-      AddrSpaceMap = &SPIRAddrSpaceMap;
+      PhysicalAddrSpaceMap = LogicalAddrSpaceMap = &SPIRAddrSpaceMap;
       // Define available target features
       // These must be defined in sorted order!
       NoAsmVariants = true;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to