Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion clang/include/clang/Sema/SemaHLSL.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,17 @@ class SemaHLSL : public SemaBase {

void diagnoseAvailabilityViolations(TranslationUnitDecl *TU);

bool initGlobalResourceDecl(VarDecl *VD);
uint32_t getNextImplicitBindingOrderID() {
return ImplicitBindingNextOrderID++;
}

bool initGlobalResourceDecl(VarDecl *VD);
bool initGlobalResourceArrayDecl(VarDecl *VD);
void createResourceRecordCtorArgs(const Type *ResourceTy, StringRef VarName,
HLSLResourceBindingAttr *RBA,
HLSLVkBindingAttr *VkBinding,
uint32_t ArrayIndex,
llvm::SmallVectorImpl<Expr *> &Args);
};

} // namespace clang
Expand Down
10 changes: 10 additions & 0 deletions clang/lib/CodeGen/CGExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "CGCall.h"
#include "CGCleanup.h"
#include "CGDebugInfo.h"
#include "CGHLSLRuntime.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
#include "CGRecordLayout.h"
Expand Down Expand Up @@ -4534,6 +4535,15 @@ LValue CodeGenFunction::EmitArraySubscriptExpr(const ArraySubscriptExpr *E,
LHS.getBaseInfo(), TBAAAccessInfo());
}

// The HLSL runtime handle the subscript expression on global resource arrays.
if (getLangOpts().HLSL && (E->getType()->isHLSLResourceRecord() ||
E->getType()->isHLSLResourceRecordArray())) {
std::optional<LValue> LV =
CGM.getHLSLRuntime().emitResourceArraySubscriptExpr(E, *this);
if (LV.has_value())
return *LV;
}

// All the other cases basically behave like simple offsetting.

// Handle the extvector case we ignored above.
Expand Down
206 changes: 194 additions & 12 deletions clang/lib/CodeGen/CGHLSLRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Type.h"
#include "clang/Basic/TargetOptions.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Frontend/HLSL/RootSignatureMetadata.h"
#include "llvm/IR/Constants.h"
Expand Down Expand Up @@ -84,6 +85,111 @@ void addRootSignature(llvm::dxbc::RootSignatureVersion RootSigVer,
RootSignatureValMD->addOperand(MDVals);
}

// Find array variable declaration from nested array subscript AST nodes
static const ValueDecl *getArrayDecl(const ArraySubscriptExpr *ASE) {
const Expr *E = nullptr;
while (ASE != nullptr) {
E = ASE->getBase()->IgnoreImpCasts();
if (!E)
return nullptr;
ASE = dyn_cast<ArraySubscriptExpr>(E);
}
if (const DeclRefExpr *DRE = dyn_cast_or_null<DeclRefExpr>(E))
return DRE->getDecl();
return nullptr;
}

// Get the total size of the array, or -1 if the array is unbounded.
static int getTotalArraySize(ASTContext &AST, const clang::Type *Ty) {
Ty = Ty->getUnqualifiedDesugaredType();
assert(Ty->isArrayType() && "expected array type");
if (Ty->isIncompleteArrayType())
return -1;
return AST.getConstantArrayElementCount(cast<ConstantArrayType>(Ty));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure Ty always will be a ConstantArrayType?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Ty should always be ConstantArrayType here. In case it is not, the assert in cast will validate that.

}

// Find constructor decl for a specific resource record type and binding
// (implicit vs. explicit). The constructor has 5 parameters.
// For explicit binding the signature is:
// void(unsigned, unsigned, int, unsigned, const char *).
// For implicit binding the signature is:
// void(unsigned, int, unsigned, unsigned, const char *).
static CXXConstructorDecl *findResourceConstructorDecl(ASTContext &AST,
QualType ResTy,
bool ExplicitBinding) {
SmallVector<QualType> ExpParmTypes = {
AST.UnsignedIntTy, AST.UnsignedIntTy, AST.UnsignedIntTy,
AST.UnsignedIntTy, AST.getPointerType(AST.CharTy.withConst())};
ExpParmTypes[ExplicitBinding ? 2 : 1] = AST.IntTy;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit subtle that the only way to differentiate these constructors is the signedness of a single argument.

It's probably more complicated in practice so maybe it isn't worth it, but did you consider using static member functions to work like named constructors? That is,

class RWBuffer<T> {
  // ...
public:
  static RWBuffer<T> fromExplicitBinding(...);
  static RWBuffer<T> fromImplicitBinding(...);
  // ...
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not considered static create functions because the constructors should never really need to be explicitly called by the user. In fact, we should probably make them private (#154221).
@s-perron will likely be adding more constructor overloads in order to support counters on raw buffers. Maybe we can revisit it then if it makes sense to do the static functions?


CXXRecordDecl *ResDecl = ResTy->getAsCXXRecordDecl();
for (auto *Ctor : ResDecl->ctors()) {
if (Ctor->getNumParams() != ExpParmTypes.size())
continue;
ParmVarDecl **ParmIt = Ctor->param_begin();
QualType *ExpTyIt = ExpParmTypes.begin();
for (; ParmIt != Ctor->param_end() && ExpTyIt != ExpParmTypes.end();
++ParmIt, ++ExpTyIt) {
if ((*ParmIt)->getType() != *ExpTyIt)
break;
}
if (ParmIt == Ctor->param_end())
return Ctor;
}
llvm_unreachable("did not find constructor for resource class");
}

static Value *buildNameForResource(llvm::StringRef BaseName,
CodeGenModule &CGM) {
llvm::SmallString<64> GlobalName = {BaseName, ".str"};
return CGM.GetAddrOfConstantCString(BaseName.str(), GlobalName.c_str())
.getPointer();
}

static void createResourceCtorArgs(CodeGenModule &CGM, CXXConstructorDecl *CD,
llvm::Value *ThisPtr, llvm::Value *Range,
llvm::Value *Index, StringRef Name,
HLSLResourceBindingAttr *RBA,
HLSLVkBindingAttr *VkBinding,
CallArgList &Args) {
assert((VkBinding || RBA) && "at least one a binding attribute expected");

std::optional<uint32_t> RegisterSlot;
uint32_t SpaceNo = 0;
if (VkBinding) {
RegisterSlot = VkBinding->getBinding();
SpaceNo = VkBinding->getSet();
} else {
if (RBA->hasRegisterSlot())
RegisterSlot = RBA->getSlotNumber();
SpaceNo = RBA->getSpaceNumber();
}

ASTContext &AST = CD->getASTContext();
Value *NameStr = buildNameForResource(Name, CGM);
Value *Space = llvm::ConstantInt::get(CGM.IntTy, SpaceNo);

Args.add(RValue::get(ThisPtr), CD->getThisType());
if (RegisterSlot.has_value()) {
// explicit binding
auto *RegSlot = llvm::ConstantInt::get(CGM.IntTy, RegisterSlot.value());
Args.add(RValue::get(RegSlot), AST.UnsignedIntTy);
Args.add(RValue::get(Space), AST.UnsignedIntTy);
Args.add(RValue::get(Range), AST.IntTy);
Args.add(RValue::get(Index), AST.UnsignedIntTy);

} else {
// implicit binding
auto *OrderID =
llvm::ConstantInt::get(CGM.IntTy, RBA->getImplicitBindingOrderID());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unconditional access of RBA looks incorrect. In the first if/else statements you know RBA is not nullptr b/c VkBinding is nullptr but in this set that is not obviously true since RegisterSlot is set in both branches of the previous if/else statement pair.

CC @llvm-beanz

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way to get to the else branch here is when RegisterSlot is not set, and that happens only when VkBinding is nullptr, which means RBA is not nullptr, and also when RBA->hasRegisterSlot() returns false.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the logic here is a bit unclear, maybe it warrants an assert to ensure that the surrounding code never changes the preconditions?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer at least some asserts documenting this b/c it is not obvious at all and it should prevent later changes from breaking those assumptions or at least allow us to catch is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an assert: #156094

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Args.add(RValue::get(Space), AST.UnsignedIntTy);
Args.add(RValue::get(Range), AST.IntTy);
Args.add(RValue::get(Index), AST.UnsignedIntTy);
Args.add(RValue::get(OrderID), AST.UnsignedIntTy);
}
Args.add(RValue::get(NameStr), AST.getPointerType(AST.CharTy.withConst()));
}

} // namespace

llvm::Type *
Expand Down Expand Up @@ -589,13 +695,6 @@ static void initializeBuffer(CodeGenModule &CGM, llvm::GlobalVariable *GV,
CGM.AddCXXGlobalInit(InitResFunc);
}

static Value *buildNameForResource(llvm::StringRef BaseName,
CodeGenModule &CGM) {
std::string Str(BaseName);
std::string GlobalName(Str + ".str");
return CGM.GetAddrOfConstantCString(Str, GlobalName.c_str()).getPointer();
}

void CGHLSLRuntime::initializeBufferFromBinding(const HLSLBufferDecl *BufDecl,
llvm::GlobalVariable *GV,
HLSLVkBindingAttr *VkBinding) {
Expand Down Expand Up @@ -623,17 +722,13 @@ void CGHLSLRuntime::initializeBufferFromBinding(const HLSLBufferDecl *BufDecl,
auto *Index = llvm::ConstantInt::get(CGM.IntTy, 0);
auto *RangeSize = llvm::ConstantInt::get(CGM.IntTy, 1);
auto *Space = llvm::ConstantInt::get(CGM.IntTy, RBA->getSpaceNumber());
Value *Name = nullptr;
Value *Name = buildNameForResource(BufDecl->getName(), CGM);

llvm::Intrinsic::ID IntrinsicID =
RBA->hasRegisterSlot()
? CGM.getHLSLRuntime().getCreateHandleFromBindingIntrinsic()
: CGM.getHLSLRuntime().getCreateHandleFromImplicitBindingIntrinsic();

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());
Expand Down Expand Up @@ -700,3 +795,90 @@ void CGHLSLRuntime::emitInitListOpaqueValues(CodeGenFunction &CGF,
}
}
}

std::optional<LValue> CGHLSLRuntime::emitResourceArraySubscriptExpr(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this function be better named? I don't think it emits a resource array subscript expr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the name is quite descriptive - it is emitting code for subscript expression on resource arrays, or in other words, emitting code for ArraySubscriptExpr AST node on a resources array. I think it matches the existing patterns of CGF.Emit*, such as CGF.EmitScalarExp or CGF.EmitCallExpr. Do you have any other suggestions?

const ArraySubscriptExpr *ArraySubsExpr, CodeGenFunction &CGF) {
assert(ArraySubsExpr->getType()->isHLSLResourceRecord() ||
ArraySubsExpr->getType()->isHLSLResourceRecordArray() &&
"expected resource array subscript expression");

// let clang codegen handle local resource array subscripts
const VarDecl *ArrayDecl = dyn_cast<VarDecl>(getArrayDecl(ArraySubsExpr));
if (!ArrayDecl || !ArrayDecl->hasGlobalStorage())
return std::nullopt;

// FIXME: this is not yet implemented (llvm/llvm-project#145426)
assert(!ArraySubsExpr->getType()->isArrayType() &&
"indexing of array subsets it not supported yet");

// get the resource array type
ASTContext &AST = ArrayDecl->getASTContext();
const Type *ResArrayTy = ArrayDecl->getType().getTypePtr();
assert(ResArrayTy->isHLSLResourceRecordArray() &&
"expected array of resource classes");

// Iterate through all nested array subscript expressions to calculate
// the index in the flattened resource array (if this is a multi-
// dimensional array). The index is calculated as a sum of all indices
// multiplied by the total size of the array at that level.
Value *Index = nullptr;
const ArraySubscriptExpr *ASE = ArraySubsExpr;
while (ASE != nullptr) {
Value *SubIndex = CGF.EmitScalarExpr(ASE->getIdx());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could SubIndex ever be null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The result of EmitScalarExpr is generally assumed to be non-null.

if (const auto *ArrayTy =
dyn_cast<ConstantArrayType>(ASE->getType().getTypePtr())) {
Value *Multiplier = llvm::ConstantInt::get(
CGM.IntTy, AST.getConstantArrayElementCount(ArrayTy));
SubIndex = CGF.Builder.CreateMul(SubIndex, Multiplier);
}

Index = Index ? CGF.Builder.CreateAdd(Index, SubIndex) : SubIndex;
ASE = dyn_cast<ArraySubscriptExpr>(ASE->getBase()->IgnoreParenImpCasts());
}

// find binding info for the resource array (for implicit binding
// an HLSLResourceBindingAttr should have been added by SemaHLSL)
QualType ResourceTy = ArraySubsExpr->getType();
HLSLVkBindingAttr *VkBinding = ArrayDecl->getAttr<HLSLVkBindingAttr>();
HLSLResourceBindingAttr *RBA = ArrayDecl->getAttr<HLSLResourceBindingAttr>();
assert((VkBinding || RBA) && "resource array must have a binding attribute");

// lookup the resource class constructor based on the resource type and
// binding
CXXConstructorDecl *CD = findResourceConstructorDecl(
AST, ResourceTy, VkBinding || RBA->hasRegisterSlot());

// create a temporary variable for the resource class instance (we need to
// return an LValue)
RawAddress TmpVar = CGF.CreateMemTemp(ResourceTy);
if (CGF.EmitLifetimeStart(TmpVar.getPointer()))
CGF.pushFullExprCleanup<CodeGenFunction::CallLifetimeEnd>(
NormalEHLifetimeMarker, TmpVar);

AggValueSlot ValueSlot = AggValueSlot::forAddr(
TmpVar, Qualifiers(), AggValueSlot::IsDestructed_t(true),
AggValueSlot::DoesNotNeedGCBarriers, AggValueSlot::IsAliased_t(false),
AggValueSlot::DoesNotOverlap);

Address ThisAddress = ValueSlot.getAddress();
llvm::Value *ThisPtr = CGF.getAsNaturalPointerTo(
ThisAddress, CD->getThisType()->getPointeeType());

// get total array size (= range size)
llvm::Value *Range =
llvm::ConstantInt::get(CGM.IntTy, getTotalArraySize(AST, ResArrayTy));

// assemble the constructor parameters
CallArgList Args;
createResourceCtorArgs(CGM, CD, ThisPtr, Range, Index, ArrayDecl->getName(),
RBA, VkBinding, Args);

// call the constructor
CGF.EmitCXXConstructorCall(CD, Ctor_Complete, false, false, ThisAddress, Args,
ValueSlot.mayOverlap(),
ArraySubsExpr->getExprLoc(),
ValueSlot.isSanitizerChecked());

return CGF.MakeAddrLValue(TmpVar, ArraySubsExpr->getType(),
AlignmentSource::Decl);
}
6 changes: 6 additions & 0 deletions clang/lib/CodeGen/CGHLSLRuntime.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,15 @@ class Type;
class RecordType;
class DeclContext;
class HLSLPackOffsetAttr;
class ArraySubscriptExpr;

class FunctionDecl;

namespace CodeGen {

class CodeGenModule;
class CodeGenFunction;
class LValue;

class CGHLSLRuntime {
public:
Expand Down Expand Up @@ -164,6 +166,10 @@ class CGHLSLRuntime {
llvm::TargetExtType *LayoutTy);
void emitInitListOpaqueValues(CodeGenFunction &CGF, InitListExpr *E);

std::optional<LValue>
emitResourceArraySubscriptExpr(const ArraySubscriptExpr *E,
CodeGenFunction &CGF);

private:
void emitBufferGlobalsAndMetadata(const HLSLBufferDecl *BufDecl,
llvm::GlobalVariable *BufGV);
Expand Down
17 changes: 9 additions & 8 deletions clang/lib/CodeGen/CodeGenModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5787,11 +5787,16 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
(D->getType()->isCUDADeviceBuiltinSurfaceType() ||
D->getType()->isCUDADeviceBuiltinTextureType());
if (getLangOpts().CUDA &&
(IsCUDASharedVar || IsCUDAShadowVar || IsCUDADeviceShadowVar))
(IsCUDASharedVar || IsCUDAShadowVar || IsCUDADeviceShadowVar)) {
Init = llvm::UndefValue::get(getTypes().ConvertTypeForMem(ASTTy));
else if (D->hasAttr<LoaderUninitializedAttr>())
} else if (getLangOpts().HLSL &&
(D->getType()->isHLSLResourceRecord() ||
D->getType()->isHLSLResourceRecordArray())) {
Init = llvm::PoisonValue::get(getTypes().ConvertType(ASTTy));
NeedsGlobalCtor = D->getType()->isHLSLResourceRecord();
} else if (D->hasAttr<LoaderUninitializedAttr>()) {
Init = llvm::UndefValue::get(getTypes().ConvertTypeForMem(ASTTy));
else if (!InitExpr) {
} else if (!InitExpr) {
// This is a tentative definition; tentative definitions are
// implicitly initialized with { 0 }.
//
Expand All @@ -5812,11 +5817,7 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
if (D->getType()->isReferenceType())
T = D->getType();

if (getLangOpts().HLSL &&
D->getType().getTypePtr()->isHLSLResourceRecord()) {
Init = llvm::PoisonValue::get(getTypes().ConvertType(ASTTy));
NeedsGlobalCtor = true;
} else if (getLangOpts().CPlusPlus) {
if (getLangOpts().CPlusPlus) {
Init = EmitNullConstant(T);
if (!IsDefinitionAvailableExternally)
NeedsGlobalCtor = true;
Expand Down
Loading
Loading