code review feedback - make the check that an intrinsic has a name argument more generic; update string ctors

Author: hekota

clang/lib/CodeGen/CGHLSLBuiltins.cpp

@@ -300,19 +300,18 @@ Value *CodeGenFunction::EmitHLSLBuiltinExpr(unsigned BuiltinID,
     Value *NonUniform =
         llvm::ConstantInt::get(llvm::Type::getInt1Ty(getLLVMContext()), false);
 
-    Intrinsic::ID IID =
+    auto [IntrinsicID, HasNameArg] =
         CGM.getHLSLRuntime().getCreateHandleFromBindingIntrinsic();
-    // SPIR-V intrinsic does not have include the resource name
-    if (IID == Intrinsic::spv_resource_handlefrombinding)
-      return Builder.CreateIntrinsic(
-          HandleTy, IID,
-          ArrayRef<Value *>{SpaceOp, RegisterOp, RangeOp, IndexOp, NonUniform});
-
-    Value *NameOp = EmitScalarExpr(E->getArg(5));
-    return Builder.CreateIntrinsic(HandleTy, IID,
-                                   ArrayRef<Value *>{SpaceOp, RegisterOp,
-                                                     RangeOp, IndexOp,
-                                                     NonUniform, NameOp});
+    if (HasNameArg) {
+      Value *NameOp = EmitScalarExpr(E->getArg(5));
+      return Builder.CreateIntrinsic(HandleTy, IntrinsicID,
+                                     ArrayRef<Value *>{SpaceOp, RegisterOp,
+                                                       RangeOp, IndexOp,
+                                                       NonUniform, NameOp});
+    }
+    return Builder.CreateIntrinsic(
+        HandleTy, IntrinsicID,
+        ArrayRef<Value *>{SpaceOp, RegisterOp, RangeOp, IndexOp, NonUniform});
   }
   case Builtin::BI__builtin_hlsl_resource_handlefromimplicitbinding: {
     llvm::Type *HandleTy = CGM.getTypes().ConvertType(E->getType());
@@ -325,19 +324,18 @@ Value *CodeGenFunction::EmitHLSLBuiltinExpr(unsigned BuiltinID,
     Value *NonUniform =
         llvm::ConstantInt::get(llvm::Type::getInt1Ty(getLLVMContext()), false);
 
-    Intrinsic::ID IID =
+    auto [IntrinsicID, HasNameArg] =
         CGM.getHLSLRuntime().getCreateHandleFromImplicitBindingIntrinsic();
-    // SPIR-V intrinsic does not include the resource name
-    if (IID == Intrinsic::spv_resource_handlefromimplicitbinding)
-      return Builder.CreateIntrinsic(
-          HandleTy, IID,
-          ArrayRef<Value *>{OrderID, SpaceOp, RangeOp, IndexOp, NonUniform});
-
-    Value *NameOp = EmitScalarExpr(E->getArg(5));
-    return Builder.CreateIntrinsic(HandleTy, IID,
-                                   ArrayRef<Value *>{OrderID, SpaceOp, RangeOp,
-                                                     IndexOp, NonUniform,
-                                                     NameOp});
+    if (HasNameArg) {
+      Value *NameOp = EmitScalarExpr(E->getArg(5));
+      return Builder.CreateIntrinsic(HandleTy, IntrinsicID,
+                                     ArrayRef<Value *>{OrderID, SpaceOp,
+                                                       RangeOp, IndexOp,
+                                                       NonUniform, NameOp});
+    }
+    return Builder.CreateIntrinsic(
+        HandleTy, IntrinsicID,
+        ArrayRef<Value *>{OrderID, SpaceOp, RangeOp, IndexOp, NonUniform});
   }
   case Builtin::BI__builtin_hlsl_all: {
     Value *Op0 = EmitScalarExpr(E->getArg(0));

clang/lib/CodeGen/CGHLSLRuntime.cpp

@@ -34,6 +34,7 @@
 #include "llvm/Support/Alignment.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
+#include <utility>
 
 using namespace clang;
 using namespace CodeGen;
@@ -235,6 +236,35 @@ static void fillPackoffsetLayout(const HLSLBufferDecl *BufDecl,
   }
 }
 
+std::pair<llvm::Intrinsic::ID, bool>
+CGHLSLRuntime::getCreateHandleFromBindingIntrinsic() {
+  switch (getArch()) {
+  case llvm::Triple::dxil:
+    return std::pair(llvm::Intrinsic::dx_resource_handlefrombinding, true);
+  case llvm::Triple::spirv:
+    return std::pair(llvm::Intrinsic::spv_resource_handlefrombinding, false);
+  default:
+    llvm_unreachable("Intrinsic resource_handlefrombinding not supported by "
+                     "target architecture");
+  }
+}
+
+std::pair<llvm::Intrinsic::ID, bool>
+CGHLSLRuntime::getCreateHandleFromImplicitBindingIntrinsic() {
+  switch (getArch()) {
+  case llvm::Triple::dxil:
+    return std::pair(llvm::Intrinsic::dx_resource_handlefromimplicitbinding,
+                     true);
+  case llvm::Triple::spirv:
+    return std::pair(llvm::Intrinsic::spv_resource_handlefromimplicitbinding,
+                     false);
+  default:
+    llvm_unreachable(
+        "Intrinsic resource_handlefromimplicitbinding not supported by "
+        "target architecture");
+  }
+}
+
 // Codegen for HLSLBufferDecl
 void CGHLSLRuntime::addBuffer(const HLSLBufferDecl *BufDecl) {
 
@@ -564,35 +594,35 @@ void CGHLSLRuntime::initializeBufferFromBinding(const HLSLBufferDecl *BufDecl,
       llvm::ConstantInt::get(CGM.IntTy, RBA ? RBA->getSpaceNumber() : 0);
   Value *Name = nullptr;
 
-  // DXIL intrinsic includes resource name
-  if (getArch() == Triple::dxil) {
-    std::string Str = std::string(BufDecl->getName());
-    std::string GlobalName = Str + ".str";
+  auto [IntrinsicID, HasNameArg] =
+      RBA->hasRegisterSlot()
+          ? CGM.getHLSLRuntime().getCreateHandleFromBindingIntrinsic()
+          : CGM.getHLSLRuntime().getCreateHandleFromImplicitBindingIntrinsic();
+
+  if (HasNameArg) {
+    std::string Str(BufDecl->getName());
+    std::string GlobalName(Str + ".str");
     Name = CGM.GetAddrOfConstantCString(Str, GlobalName.c_str()).getPointer();
   }
 
   // buffer with explicit binding
   if (RBA->hasRegisterSlot()) {
     auto *RegSlot = llvm::ConstantInt::get(CGM.IntTy, RBA->getSlotNumber());
-    Intrinsic::ID Intr =
-        CGM.getHLSLRuntime().getCreateHandleFromBindingIntrinsic();
     if (Name)
-      initializeBuffer(CGM, GV, Intr,
+      initializeBuffer(CGM, GV, IntrinsicID,
                        {Space, RegSlot, RangeSize, Index, NonUniform, Name});
     else
-      initializeBuffer(CGM, GV, Intr,
+      initializeBuffer(CGM, GV, IntrinsicID,
                        {Space, RegSlot, RangeSize, Index, NonUniform});
   } else {
     // buffer with implicit binding
     auto *OrderID =
         llvm::ConstantInt::get(CGM.IntTy, RBA->getImplicitBindingOrderID());
-    Intrinsic::ID Intr =
-        CGM.getHLSLRuntime().getCreateHandleFromImplicitBindingIntrinsic();
     if (Name)
-      initializeBuffer(CGM, GV, Intr,
+      initializeBuffer(CGM, GV, IntrinsicID,
                        {OrderID, Space, RangeSize, Index, NonUniform, Name});
     else
-      initializeBuffer(CGM, GV, Intr,
+      initializeBuffer(CGM, GV, IntrinsicID,
                        {OrderID, Space, RangeSize, Index, NonUniform});
   }
 }

clang/lib/CodeGen/CGHLSLRuntime.h

@@ -117,10 +117,6 @@ class CGHLSLRuntime {
 
   GENERATE_HLSL_INTRINSIC_FUNCTION(CreateResourceGetPointer,
                                    resource_getpointer)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(CreateHandleFromBinding,
-                                   resource_handlefrombinding)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(CreateHandleFromImplicitBinding,
-                                   resource_handlefromimplicitbinding)
   GENERATE_HLSL_INTRINSIC_FUNCTION(BufferUpdateCounter, resource_updatecounter)
   GENERATE_HLSL_INTRINSIC_FUNCTION(GroupMemoryBarrierWithGroupSync,
                                    group_memory_barrier_with_group_sync)
@@ -129,6 +125,10 @@ class CGHLSLRuntime {
   // End of reserved area for HLSL intrinsic getters.
   //===----------------------------------------------------------------------===//
 
+  std::pair<llvm::Intrinsic::ID, bool> getCreateHandleFromBindingIntrinsic();
+  std::pair<llvm::Intrinsic::ID, bool>
+  getCreateHandleFromImplicitBindingIntrinsic();
+
 protected:
   CodeGenModule &CGM;