Repository: shader-slang/slang
File: source/slang/slang-ir-spirv-legalize.cpp (C++)

Code before:
        if (auto targetIntrinsic = Slang::findBestTargetIntrinsicDecoration(
                funcValue,
                m_sharedContext->m_targetRequest->getTargetCaps()))
        {
            SpvSnippet* snippet = m_sharedContext->getParsedSpvSnippet(targetIntrinsic);
            if (!snippet)
                return;
            if (snippet->resultStorageClass != SpvStorageClassMax)
                SLANG_UNIMPLEMENTED_X(
                    "Specifying storage classes in spirv __target_intrinsic snippets");
            return;
        }

        // According to SPIRV spec, the if the operands of a call has pointer
        // type, then it can only be a memory-object. This means that if the
        // pointer is a result of `getElementPtr`, we cannot use it as an
        // argument. In this case, we have to allocate a temp var to pass the
        // value, and write them back to the original pointer after the call.
        //
        // > SPIRV Spec section 2.16.1:
        // >   - Any pointer operand to an OpFunctionCall must be a memory object
        // >     declaration, or
        // >     - a pointer to an element in an array that is a memory object
        // >       declaration, where the element type is OpTypeSampler or OpTypeImage.
        // >   However, this restriction is removed for Workgroup and StorageBuffer if
        //     VariablePointers or VariablePointersStorageBuffer is declared.
        List<IRInst*> newArgs;
        struct WriteBackPair
        {
            IRInst* originalAddrArg;
            IRInst* tempVar;
        };
        List<WriteBackPair> writeBacks;
        IRBuilder builder(inst);
        builder.setInsertBefore(inst);
        auto funcType = as<IRFuncType>(funcValue->getFullType());
        bool argsChanged = false;
        for (UInt i = 0; i < inst->getArgCount(); i++)
        {
            auto arg = inst->getArg(i);
            auto paramType = funcType->getParamType(i);
            if (auto ptrType = as<IRPtrType>(paramType))
            {
                if (ptrType->getAddressSpace() == AddressSpace::UserPointer ||
                    ptrType->getAddressSpace() == AddressSpace::GroupShared)
                {
                    // If the parameter has an explicit pointer type,
                    // then we know the user is using the variable pointer
                    // capability to pass a true pointer.
                    // In this case we should not rewrite the call.
                    newArgs.add(arg);

Review comment (suggestion):
I don't think this is a right way to add the information.
The current comment uses `>` as a mean to say it is a quote from the spec document word-by-word.

I suggest a format like the following,


// However, this restriction is removed for Workgroup and StorageBuffer if
// VariablePointers or VariablePointersStorageBuffer is declared.
// > If the VariablePointers or VariablePointersStorageBuffer [capability](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#Capability) is declared, (...)
// > For pointer operands to [OpFunctionCall](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpFunctionCall), the [memory object declaration](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#MemoryObjectDeclaration)-restriction is removed for the following [storage classes](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#Storage_Class):
// >   - StorageBuffer
// >   - Workgroup


Code after:
        if (auto targetIntrinsic = Slang::findBestTargetIntrinsicDecoration(
                funcValue,
                m_sharedContext->m_targetRequest->getTargetCaps()))
        {
            SpvSnippet* snippet = m_sharedContext->getParsedSpvSnippet(targetIntrinsic);
            if (!snippet)
                return;
            if (snippet->resultStorageClass != SpvStorageClassMax)
                SLANG_UNIMPLEMENTED_X(
                    "Specifying storage classes in spirv __target_intrinsic snippets");
            return;
        }

        // According to SPIRV spec, the if the operands of a call has pointer
        // type, then it can only be a memory-object. This means that if the
        // pointer is a result of `getElementPtr`, we cannot use it as an
        // argument. In this case, we have to allocate a temp var to pass the
        // value, and write them back to the original pointer after the call.
        //
        // > SPIRV Spec section 2.16.1:
        // >   - Any pointer operand to an OpFunctionCall must be a memory object
        // >     declaration, or
        // >     - a pointer to an element in an array that is a memory object
        // >       declaration, where the element type is OpTypeSampler or OpTypeImage.
        //
        // However, this restriction is removed for Workgroup and StorageBuffer if
        // VariablePointers or VariablePointersStorageBuffer is declared.
        // > If the VariablePointers or VariablePointersStorageBuffer capability is declared, (...)
        // > For pointer operands to OpFunctionCall, the memory object declaration-restriction is
        // > removed for the following storage classes:
        // > - StorageBuffer
        // > - Workgroup
        List<IRInst*> newArgs;
        struct WriteBackPair
        {
            IRInst* originalAddrArg;
            IRInst* tempVar;
        };
        List<WriteBackPair> writeBacks;
        IRBuilder builder(inst);
        builder.setInsertBefore(inst);
        auto funcType = as<IRFuncType>(funcValue->getFullType());
        bool argsChanged = false;
        for (UInt i = 0; i < inst->getArgCount(); i++)
        {
            auto arg = inst->getArg(i);
            auto paramType = funcType->getParamType(i);
            if (auto ptrType = as<IRPtrType>(paramType))
            {
                if (ptrType->getAddressSpace() == AddressSpace::UserPointer ||
                    ptrType->getAddressSpace() == AddressSpace::GroupShared)