From d78ea4b0df6791fde762a28fda510c258bec6f1a Mon Sep 17 00:00:00 2001 From: Julius Ikkala Date: Sun, 12 Jan 2025 03:38:03 +0200 Subject: [PATCH 1/4] Fix nullptr in generic specialization --- source/slang/slang-ir-clone.cpp | 20 ++--- source/slang/slang-ir-insts.h | 1 + source/slang/slang-ir-link.cpp | 9 +++ source/slang/slang-ir.cpp | 139 +++++++++++++++++--------------- 4 files changed, 94 insertions(+), 75 deletions(-) diff --git a/source/slang/slang-ir-clone.cpp b/source/slang/slang-ir-clone.cpp index 6bbaefa262..1f84838e0b 100644 --- a/source/slang/slang-ir-clone.cpp +++ b/source/slang/slang-ir-clone.cpp @@ -51,16 +51,16 @@ IRInst* cloneInstAndOperands(IRCloneEnv* env, IRBuilder* builder, IRInst* oldIns SLANG_ASSERT(builder); SLANG_ASSERT(oldInst); - // This logic will not handle any instructions - // with special-case data attached, but that only - // applies to `IRConstant`s at this point, and those - // should only appear at the global scope rather than - // in function bodies. - // - // TODO: It would be easy enough to extend this logic - // to handle constants gracefully, if it ever comes up. - // - SLANG_ASSERT(!as(oldInst)); + // IRConstants contain data other than just the operands, so cloning them + // is done separately. + IRConstant* oldConstant = as(oldInst); + if (oldConstant) + { + IRConstant* newConstant = builder->getClonedConstantValue(*oldConstant); + builder->addInst(newConstant); + newConstant->sourceLoc = oldInst->sourceLoc; + return newConstant; + } // We start by mapping the type of the orignal instruction // to its replacement value, if any. diff --git a/source/slang/slang-ir-insts.h b/source/slang/slang-ir-insts.h index a58c2e900c..4d98cb221b 100644 --- a/source/slang/slang-ir-insts.h +++ b/source/slang/slang-ir-insts.h @@ -3601,6 +3601,7 @@ struct IRBuilder IRPtrLit* getNullVoidPtrValue() { return getNullPtrValue(getPtrType(getVoidType())); } IRVoidLit* getVoidValue(); IRInst* getCapabilityValue(CapabilitySet const& caps); + IRConstant* getClonedConstantValue(IRConstant& value); IRBasicType* getBasicType(BaseType baseType); IRBasicType* getVoidType(); diff --git a/source/slang/slang-ir-link.cpp b/source/slang/slang-ir-link.cpp index d60903cfc2..364e58c48e 100644 --- a/source/slang/slang-ir-link.cpp +++ b/source/slang/slang-ir-link.cpp @@ -1172,6 +1172,15 @@ IRInst* cloneInst( { // We need to special-case any instruction that is not // allocated like an ordinary `IRInst` with trailing args. + case kIROp_IntLit: + case kIROp_FloatLit: + case kIROp_BoolLit: + case kIROp_StringLit: + case kIROp_BlobLit: + case kIROp_PtrLit: + case kIROp_VoidLit: + return cloneValue(context, originalInst); + case kIROp_Func: return cloneFuncImpl(context, builder, cast(originalInst), originalValues); diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index e15ec6f07b..65fc05320b 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -2192,71 +2192,7 @@ IRConstant* IRBuilder::_findOrEmitConstant(IRConstant& keyInst) return irValue; } - // Calculate the minimum object size (ie not including the payload of value) - const size_t prefixSize = SLANG_OFFSET_OF(IRConstant, value); - - switch (keyInst.getOp()) - { - default: - SLANG_UNEXPECTED("missing case for IR constant"); - break; - - case kIROp_BoolLit: - case kIROp_IntLit: - { - const size_t instSize = prefixSize + sizeof(IRIntegerValue); - irValue = static_cast( - _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); - irValue->value.intVal = keyInst.value.intVal; - break; - } - case kIROp_FloatLit: - { - const size_t instSize = prefixSize + sizeof(IRFloatingPointValue); - irValue = static_cast( - _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); - irValue->value.floatVal = keyInst.value.floatVal; - break; - } - case kIROp_PtrLit: - { - const size_t instSize = prefixSize + sizeof(void*); - irValue = static_cast( - _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); - irValue->value.ptrVal = keyInst.value.ptrVal; - break; - } - case kIROp_VoidLit: - { - const size_t instSize = prefixSize + sizeof(void*); - irValue = static_cast( - _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); - irValue->value.ptrVal = keyInst.value.ptrVal; - break; - } - case kIROp_BlobLit: - case kIROp_StringLit: - { - const UnownedStringSlice slice = keyInst.getStringSlice(); - - const size_t sliceSize = slice.getLength(); - const size_t instSize = - prefixSize + offsetof(IRConstant::StringValue, chars) + sliceSize; - - irValue = static_cast( - _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); - - IRConstant::StringValue& dstString = irValue->value.stringVal; - - dstString.numChars = uint32_t(sliceSize); - // Turn into pointer to avoid warning of array overrun - char* dstChars = dstString.chars; - // Copy the chars - memcpy(dstChars, slice.begin(), sliceSize); - - break; - } - } + irValue = getClonedConstantValue(keyInst); key.inst = irValue; m_dedupContext->getConstantMap().add(key, irValue); @@ -2469,6 +2405,79 @@ IRInst* IRBuilder::getCapabilityValue(CapabilitySet const& caps) conjunctions.getBuffer()); } +IRConstant* IRBuilder::getClonedConstantValue(IRConstant& value) +{ + IRConstant* irValue = nullptr; + + // Calculate the minimum object size (ie not including the payload of value) + const size_t prefixSize = SLANG_OFFSET_OF(IRConstant, value); + + switch (value.getOp()) + { + default: + SLANG_UNEXPECTED("missing case for IR constant"); + break; + + case kIROp_BoolLit: + case kIROp_IntLit: + { + const size_t instSize = prefixSize + sizeof(IRIntegerValue); + irValue = static_cast( + _createInst(instSize, value.getFullType(), value.getOp())); + irValue->value.intVal = value.value.intVal; + break; + } + case kIROp_FloatLit: + { + const size_t instSize = prefixSize + sizeof(IRFloatingPointValue); + irValue = static_cast( + _createInst(instSize, value.getFullType(), value.getOp())); + irValue->value.floatVal = value.value.floatVal; + break; + } + case kIROp_PtrLit: + { + const size_t instSize = prefixSize + sizeof(void*); + irValue = static_cast( + _createInst(instSize, value.getFullType(), value.getOp())); + irValue->value.ptrVal = value.value.ptrVal; + break; + } + case kIROp_VoidLit: + { + const size_t instSize = prefixSize + sizeof(void*); + irValue = static_cast( + _createInst(instSize, value.getFullType(), value.getOp())); + irValue->value.ptrVal = value.value.ptrVal; + break; + } + case kIROp_BlobLit: + case kIROp_StringLit: + { + const UnownedStringSlice slice = value.getStringSlice(); + + const size_t sliceSize = slice.getLength(); + const size_t instSize = + prefixSize + offsetof(IRConstant::StringValue, chars) + sliceSize; + + irValue = static_cast( + _createInst(instSize, value.getFullType(), value.getOp())); + + IRConstant::StringValue& dstString = irValue->value.stringVal; + + dstString.numChars = uint32_t(sliceSize); + // Turn into pointer to avoid warning of array overrun + char* dstChars = dstString.chars; + // Copy the chars + memcpy(dstChars, slice.begin(), sliceSize); + + break; + } + } + + return irValue; +} + static void canonicalizeInstOperands(IRBuilder& builder, IROp op, ArrayView operands) { // For Array types, we always want to make sure its element count From 191e7956fd7ebc44ed68cb75354aeb6b309b3f98 Mon Sep 17 00:00:00 2001 From: Julius Ikkala Date: Sun, 12 Jan 2025 13:49:42 +0200 Subject: [PATCH 2/4] Fix formatting --- source/slang/slang-ir.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 65fc05320b..5dadfaa2e8 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -2422,32 +2422,32 @@ IRConstant* IRBuilder::getClonedConstantValue(IRConstant& value) case kIROp_IntLit: { const size_t instSize = prefixSize + sizeof(IRIntegerValue); - irValue = static_cast( - _createInst(instSize, value.getFullType(), value.getOp())); + irValue = + static_cast(_createInst(instSize, value.getFullType(), value.getOp())); irValue->value.intVal = value.value.intVal; break; } case kIROp_FloatLit: { const size_t instSize = prefixSize + sizeof(IRFloatingPointValue); - irValue = static_cast( - _createInst(instSize, value.getFullType(), value.getOp())); + irValue = + static_cast(_createInst(instSize, value.getFullType(), value.getOp())); irValue->value.floatVal = value.value.floatVal; break; } case kIROp_PtrLit: { const size_t instSize = prefixSize + sizeof(void*); - irValue = static_cast( - _createInst(instSize, value.getFullType(), value.getOp())); + irValue = + static_cast(_createInst(instSize, value.getFullType(), value.getOp())); irValue->value.ptrVal = value.value.ptrVal; break; } case kIROp_VoidLit: { const size_t instSize = prefixSize + sizeof(void*); - irValue = static_cast( - _createInst(instSize, value.getFullType(), value.getOp())); + irValue = + static_cast(_createInst(instSize, value.getFullType(), value.getOp())); irValue->value.ptrVal = value.value.ptrVal; break; } @@ -2460,8 +2460,8 @@ IRConstant* IRBuilder::getClonedConstantValue(IRConstant& value) const size_t instSize = prefixSize + offsetof(IRConstant::StringValue, chars) + sliceSize; - irValue = static_cast( - _createInst(instSize, value.getFullType(), value.getOp())); + irValue = + static_cast(_createInst(instSize, value.getFullType(), value.getOp())); IRConstant::StringValue& dstString = irValue->value.stringVal; From 2fc210fdd51cfa1307711481e7ceeeac0cbde755 Mon Sep 17 00:00:00 2001 From: Julius Ikkala Date: Mon, 13 Jan 2025 20:27:18 +0200 Subject: [PATCH 3/4] Revert "Fix nullptr in generic specialization" and add emitPtrLit instead --- source/slang/slang-ir-clone.cpp | 26 ++++-- source/slang/slang-ir-insts.h | 3 +- source/slang/slang-ir.cpp | 149 ++++++++++++++++---------------- 3 files changed, 95 insertions(+), 83 deletions(-) diff --git a/source/slang/slang-ir-clone.cpp b/source/slang/slang-ir-clone.cpp index 1f84838e0b..cafd8ab57e 100644 --- a/source/slang/slang-ir-clone.cpp +++ b/source/slang/slang-ir-clone.cpp @@ -51,17 +51,27 @@ IRInst* cloneInstAndOperands(IRCloneEnv* env, IRBuilder* builder, IRInst* oldIns SLANG_ASSERT(builder); SLANG_ASSERT(oldInst); - // IRConstants contain data other than just the operands, so cloning them - // is done separately. - IRConstant* oldConstant = as(oldInst); - if (oldConstant) + // Pointer literals need to be handled separately, as they carry other data + // than just the operands. + if (oldInst->getOp() == kIROp_PtrLit) { - IRConstant* newConstant = builder->getClonedConstantValue(*oldConstant); - builder->addInst(newConstant); - newConstant->sourceLoc = oldInst->sourceLoc; - return newConstant; + auto oldPtr = as(oldInst); + auto newInst = builder->emitPtrLit(oldPtr->getFullType(), oldPtr->value.ptrVal); + newInst->sourceLoc = oldPtr->sourceLoc; + return newInst; } + // This logic will not handle any instructions + // with special-case data attached, but that only + // applies to `IRConstant`s at this point, and those + // should only appear at the global scope rather than + // in function bodies. + // + // TODO: It would be easy enough to extend this logic + // to handle constants gracefully, if it ever comes up. + // + SLANG_ASSERT(!as(oldInst)); + // We start by mapping the type of the orignal instruction // to its replacement value, if any. // diff --git a/source/slang/slang-ir-insts.h b/source/slang/slang-ir-insts.h index 4d98cb221b..18546dfc3b 100644 --- a/source/slang/slang-ir-insts.h +++ b/source/slang/slang-ir-insts.h @@ -3601,7 +3601,6 @@ struct IRBuilder IRPtrLit* getNullVoidPtrValue() { return getNullPtrValue(getPtrType(getVoidType())); } IRVoidLit* getVoidValue(); IRInst* getCapabilityValue(CapabilitySet const& caps); - IRConstant* getClonedConstantValue(IRConstant& value); IRBasicType* getBasicType(BaseType baseType); IRBasicType* getVoidType(); @@ -4434,6 +4433,8 @@ struct IRBuilder IRGlobalConstant* emitGlobalConstant(IRType* type, IRInst* val); + IRConstant* emitPtrLit(IRType* type, void* ptr); + IRInst* emitWaveMaskBallot(IRType* type, IRInst* mask, IRInst* condition); IRInst* emitWaveMaskMatch(IRType* type, IRInst* mask, IRInst* value); diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 5dadfaa2e8..97eb55da88 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -2192,7 +2192,71 @@ IRConstant* IRBuilder::_findOrEmitConstant(IRConstant& keyInst) return irValue; } - irValue = getClonedConstantValue(keyInst); + // Calculate the minimum object size (ie not including the payload of value) + const size_t prefixSize = SLANG_OFFSET_OF(IRConstant, value); + + switch (keyInst.getOp()) + { + default: + SLANG_UNEXPECTED("missing case for IR constant"); + break; + + case kIROp_BoolLit: + case kIROp_IntLit: + { + const size_t instSize = prefixSize + sizeof(IRIntegerValue); + irValue = static_cast( + _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); + irValue->value.intVal = keyInst.value.intVal; + break; + } + case kIROp_FloatLit: + { + const size_t instSize = prefixSize + sizeof(IRFloatingPointValue); + irValue = static_cast( + _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); + irValue->value.floatVal = keyInst.value.floatVal; + break; + } + case kIROp_PtrLit: + { + const size_t instSize = prefixSize + sizeof(void*); + irValue = static_cast( + _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); + irValue->value.ptrVal = keyInst.value.ptrVal; + break; + } + case kIROp_VoidLit: + { + const size_t instSize = prefixSize + sizeof(void*); + irValue = static_cast( + _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); + irValue->value.ptrVal = keyInst.value.ptrVal; + break; + } + case kIROp_BlobLit: + case kIROp_StringLit: + { + const UnownedStringSlice slice = keyInst.getStringSlice(); + + const size_t sliceSize = slice.getLength(); + const size_t instSize = + prefixSize + offsetof(IRConstant::StringValue, chars) + sliceSize; + + irValue = static_cast( + _createInst(instSize, keyInst.getFullType(), keyInst.getOp())); + + IRConstant::StringValue& dstString = irValue->value.stringVal; + + dstString.numChars = uint32_t(sliceSize); + // Turn into pointer to avoid warning of array overrun + char* dstChars = dstString.chars; + // Copy the chars + memcpy(dstChars, slice.begin(), sliceSize); + + break; + } + } key.inst = irValue; m_dedupContext->getConstantMap().add(key, irValue); @@ -2405,79 +2469,6 @@ IRInst* IRBuilder::getCapabilityValue(CapabilitySet const& caps) conjunctions.getBuffer()); } -IRConstant* IRBuilder::getClonedConstantValue(IRConstant& value) -{ - IRConstant* irValue = nullptr; - - // Calculate the minimum object size (ie not including the payload of value) - const size_t prefixSize = SLANG_OFFSET_OF(IRConstant, value); - - switch (value.getOp()) - { - default: - SLANG_UNEXPECTED("missing case for IR constant"); - break; - - case kIROp_BoolLit: - case kIROp_IntLit: - { - const size_t instSize = prefixSize + sizeof(IRIntegerValue); - irValue = - static_cast(_createInst(instSize, value.getFullType(), value.getOp())); - irValue->value.intVal = value.value.intVal; - break; - } - case kIROp_FloatLit: - { - const size_t instSize = prefixSize + sizeof(IRFloatingPointValue); - irValue = - static_cast(_createInst(instSize, value.getFullType(), value.getOp())); - irValue->value.floatVal = value.value.floatVal; - break; - } - case kIROp_PtrLit: - { - const size_t instSize = prefixSize + sizeof(void*); - irValue = - static_cast(_createInst(instSize, value.getFullType(), value.getOp())); - irValue->value.ptrVal = value.value.ptrVal; - break; - } - case kIROp_VoidLit: - { - const size_t instSize = prefixSize + sizeof(void*); - irValue = - static_cast(_createInst(instSize, value.getFullType(), value.getOp())); - irValue->value.ptrVal = value.value.ptrVal; - break; - } - case kIROp_BlobLit: - case kIROp_StringLit: - { - const UnownedStringSlice slice = value.getStringSlice(); - - const size_t sliceSize = slice.getLength(); - const size_t instSize = - prefixSize + offsetof(IRConstant::StringValue, chars) + sliceSize; - - irValue = - static_cast(_createInst(instSize, value.getFullType(), value.getOp())); - - IRConstant::StringValue& dstString = irValue->value.stringVal; - - dstString.numChars = uint32_t(sliceSize); - // Turn into pointer to avoid warning of array overrun - char* dstChars = dstString.chars; - // Copy the chars - memcpy(dstChars, slice.begin(), sliceSize); - - break; - } - } - - return irValue; -} - static void canonicalizeInstOperands(IRBuilder& builder, IROp op, ArrayView operands) { // For Array types, we always want to make sure its element count @@ -5853,6 +5844,16 @@ IRGlobalConstant* IRBuilder::emitGlobalConstant(IRType* type, IRInst* val) return inst; } +IRConstant* IRBuilder::emitPtrLit(IRType* type, void* ptrVal) +{ + const size_t prefixSize = SLANG_OFFSET_OF(IRConstant, value); + const size_t instSize = prefixSize + sizeof(void*); + auto inst = static_cast(_createInst(instSize, type, kIROp_PtrLit)); + inst->value.ptrVal = ptrVal; + addInst(inst); + return inst; +} + IRInst* IRBuilder::emitWaveMaskBallot(IRType* type, IRInst* mask, IRInst* condition) { auto inst = createInst(this, kIROp_WaveMaskBallot, type, mask, condition); From e7854c2b78e18eb5dc6a7a6546d0240bb72b8af9 Mon Sep 17 00:00:00 2001 From: Julius Ikkala Date: Wed, 15 Jan 2025 00:50:37 +0200 Subject: [PATCH 4/4] Add type parameter to getPtrValue() --- source/slang/slang-ir-clone.cpp | 16 +++++++--------- source/slang/slang-ir-insts.h | 4 +--- source/slang/slang-ir.cpp | 15 ++------------- 3 files changed, 10 insertions(+), 25 deletions(-) diff --git a/source/slang/slang-ir-clone.cpp b/source/slang/slang-ir-clone.cpp index cafd8ab57e..66af405d6f 100644 --- a/source/slang/slang-ir-clone.cpp +++ b/source/slang/slang-ir-clone.cpp @@ -51,14 +51,18 @@ IRInst* cloneInstAndOperands(IRCloneEnv* env, IRBuilder* builder, IRInst* oldIns SLANG_ASSERT(builder); SLANG_ASSERT(oldInst); + // We start by mapping the type of the orignal instruction + // to its replacement value, if any. + // + auto oldType = oldInst->getFullType(); + auto newType = (IRType*)findCloneForOperand(env, oldType); + // Pointer literals need to be handled separately, as they carry other data // than just the operands. if (oldInst->getOp() == kIROp_PtrLit) { auto oldPtr = as(oldInst); - auto newInst = builder->emitPtrLit(oldPtr->getFullType(), oldPtr->value.ptrVal); - newInst->sourceLoc = oldPtr->sourceLoc; - return newInst; + return builder->getPtrValue(newType, oldPtr->value.ptrVal); } // This logic will not handle any instructions @@ -72,12 +76,6 @@ IRInst* cloneInstAndOperands(IRCloneEnv* env, IRBuilder* builder, IRInst* oldIns // SLANG_ASSERT(!as(oldInst)); - // We start by mapping the type of the orignal instruction - // to its replacement value, if any. - // - auto oldType = oldInst->getFullType(); - auto newType = (IRType*)findCloneForOperand(env, oldType); - // Next we will iterate over the operands of `oldInst` // to find their replacements and install them as // the operands of `newInst`. diff --git a/source/slang/slang-ir-insts.h b/source/slang/slang-ir-insts.h index 18546dfc3b..f187517140 100644 --- a/source/slang/slang-ir-insts.h +++ b/source/slang/slang-ir-insts.h @@ -3596,7 +3596,7 @@ struct IRBuilder IRInst* getFloatValue(IRType* type, IRFloatingPointValue value); IRStringLit* getStringValue(const UnownedStringSlice& slice); IRBlobLit* getBlobValue(ISlangBlob* blob); - IRPtrLit* _getPtrValue(void* ptr); + IRPtrLit* getPtrValue(IRType* type, void* ptr); IRPtrLit* getNullPtrValue(IRType* type); IRPtrLit* getNullVoidPtrValue() { return getNullPtrValue(getPtrType(getVoidType())); } IRVoidLit* getVoidValue(); @@ -4433,8 +4433,6 @@ struct IRBuilder IRGlobalConstant* emitGlobalConstant(IRType* type, IRInst* val); - IRConstant* emitPtrLit(IRType* type, void* ptr); - IRInst* emitWaveMaskBallot(IRType* type, IRInst* mask, IRInst* condition); IRInst* emitWaveMaskMatch(IRType* type, IRInst* mask, IRInst* value); diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index 97eb55da88..ac51ae451d 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -2395,9 +2395,8 @@ IRBlobLit* IRBuilder::getBlobValue(ISlangBlob* blob) return static_cast(_findOrEmitConstant(keyInst)); } -IRPtrLit* IRBuilder::_getPtrValue(void* data) +IRPtrLit* IRBuilder::getPtrValue(IRType* type, void* data) { - auto type = getPtrType(getVoidType()); IRConstant keyInst; memset(&keyInst, 0, sizeof(keyInst)); keyInst.m_op = kIROp_PtrLit; @@ -5844,16 +5843,6 @@ IRGlobalConstant* IRBuilder::emitGlobalConstant(IRType* type, IRInst* val) return inst; } -IRConstant* IRBuilder::emitPtrLit(IRType* type, void* ptrVal) -{ - const size_t prefixSize = SLANG_OFFSET_OF(IRConstant, value); - const size_t instSize = prefixSize + sizeof(void*); - auto inst = static_cast(_createInst(instSize, type, kIROp_PtrLit)); - inst->value.ptrVal = ptrVal; - addInst(inst); - return inst; -} - IRInst* IRBuilder::emitWaveMaskBallot(IRType* type, IRInst* mask, IRInst* condition) { auto inst = createInst(this, kIROp_WaveMaskBallot, type, mask, condition); @@ -6334,7 +6323,7 @@ IRDecoration* IRBuilder::addDecoration( void IRBuilder::addHighLevelDeclDecoration(IRInst* inst, Decl* decl) { - auto ptrConst = _getPtrValue(decl); + auto ptrConst = getPtrValue(getPtrType(getVoidType()), decl); addDecoration(inst, kIROp_HighLevelDeclDecoration, ptrConst); }