counter var3 #162291
counter var3 #162291
Conversation
The attribute needs to be preserved for rewriter scenarios. Two places are updated to use the ResourceBindingAttrs helper struct to make sure the HLSLVkBindingAttr is ignored when the target is DirectX.
|
@luciechoi Can you take a look at this PR? |
This commit implements the Sema and CodeGen portions of the typed buffer counter proposal described in the HLSL WG proposal 0023. This change introduces the necessary Sema and CodeGen logic to handle implicit counter variables for typed buffers. This includes: - Extending `HLSLResourceBindingAttr` to store the implicit counter binding order ID. - Introducing the `__builtin_hlsl_resource_counterhandlefromimplicitbinding` builtin. - Updating `SemaHLSL` to correctly initialize global resource declarations and resource arrays with implicit counter buffers. - Adding CodeGen support for the new builtin, which generates a `llvm.spv.resource.counterhandlefromimplicitbinding` intrinsic for the SPIR-V target and aliases the main resource handle for the DXIL target. - Adding and updating tests to verify the new functionality. Fixes llvm#137032
|
LGTM |
| llvm::Type *HandleTy = CGM.getTypes().ConvertType(E->getType()); | ||
| Value *MainHandle = EmitScalarExpr(E->getArg(0)); | ||
| if (CGM.getTriple().isSPIRV()) { |
There was a problem hiding this comment.
HandleTy is used only for the SPIR-V case and the conversion can be moved inside the if statement. Also, consider early exit for the DXIL case:
if (!CGM.getTriple().isSPIRV())
return MainHandle;
| if (Binding.hasCounterImplicitOrderID()) | ||
| CreateMethod = lookupMethod( | ||
| ResourceDecl, "__createFromBindingWithImplicitCounter", SC_Static); | ||
| else | ||
| CreateMethod = | ||
| lookupMethod(ResourceDecl, "__createFromBinding", SC_Static); |
There was a problem hiding this comment.
How about this?
const char *Name = Binding.hasCounterImplicitOrderID()
? "__createFromBindingWithImplicitCounter"
: "__createFromBinding";
CreateMethod = lookupMethod(ResourceDecl, Name, SC_Static);
| if (Record->isCompleteDefinition()) | ||
| return *this; |
There was a problem hiding this comment.
| if (Record->isCompleteDefinition()) | |
| return *this; | |
| assert(!Record->isCompleteDefinition() && "record is already complete"); |
I've recently change these to asserts because the record should never be complete here.
| .finalize(); | ||
| } | ||
|
|
||
| BuiltinTypeDeclBuilder & |
There was a problem hiding this comment.
Could you please add a comment here and to the other new static method describing what the method looks like, similar to what the existing static create methods have? See line 867.
| if (Record->isCompleteDefinition()) | ||
| return *this; |
There was a problem hiding this comment.
| if (Record->isCompleteDefinition()) | |
| return *this; | |
| assert(!Record->isCompleteDefinition() && "record is already complete"); |
| // Re-create the binding object to pick up the new attribute. | ||
| Binding = ResourceBindingAttrs(VD); |
There was a problem hiding this comment.
This is needed only when a new attribute was added, so it can be moved to the else branch right after addImplicitBindingAttrToDecl.
| llvm::SmallVector<Expr *> Args; | ||
|
|
||
| bool HasCounter = hasCounterHandle(ResourceDecl); | ||
| std::string CreateMethodName; |
There was a problem hiding this comment.
| std::string CreateMethodName; | |
| const char *CreateMethodName; |
Using const char * will prevent allocation and copying of the string.
| if (!CreateMethod) { | ||
| llvm::dbgs() << "STEVEN: failed to get method " << CreateMethodName << "\n"; | ||
| VD->dumpColor(); | ||
| } |
There was a problem hiding this comment.
Don't forget to remove before merging :)
| Args.push_back(NameCast); | ||
|
|
||
| if (HasCounter) { | ||
| // Will this be in the correct order? |
There was a problem hiding this comment.
Yes, it will be in the correct order. initGlobalResourceDecl is called from ActOnUninitializedVarDecl, which follows shortly after 'ActOnVariableDeclarator`. There should not be any other declarations processed in-between these two calls.
| // CHECK-NEXT: call void @hlsl::AppendStructuredBuffer<float>::AppendStructuredBuffer()(ptr {{.*}} %Buf3) | ||
|
|
||
| // Buf3 initialization part 2 - body of AppendStructuredBuffer<float> default C1 constructor that calls | ||
| // the default C2 constructor | ||
| // CHECK: define linkonce_odr hidden void @hlsl::StructuredBuffer<float>::StructuredBuffer()(ptr {{.*}} %this) |
There was a problem hiding this comment.
// Buf3 initialization part 1
Has this been removed intentionally? It should probably stay in, although the // CHECK: following it seems to be incorrectly refering to @hlsl::StructuredBuffer<float>::StructuredBuffer().
[SPIRV][HLSL] Add Sema and CodeGen for implicit typed buffer counters
This commit implements the Sema and CodeGen portions of the typed buffer
counter proposal described in the HLSL WG proposal 0023.
This change introduces the necessary Sema and CodeGen logic to handle
implicit counter variables for typed buffers. This includes:
HLSLResourceBindingAttrto store the implicit counterbinding order ID.
__builtin_hlsl_resource_counterhandlefromimplicitbindingbuiltin.
SemaHLSLto correctly initialize global resource declarationsand resource arrays with implicit counter buffers.
llvm.spv.resource.counterhandlefromimplicitbindingintrinsic for theSPIR-V target and aliases the main resource handle for the DXIL target.
Fixes #137032