From 9646ba85a1aeb98a76e18a1430289cd4472840ff Mon Sep 17 00:00:00 2001 From: "Sami Kiminki (NVIDIA)" <235843927+skiminki-nv@users.noreply.github.com> Date: Fri, 21 Nov 2025 18:50:56 +0200 Subject: [PATCH 1/6] Add post-fixup phase in rewriteValueUsesToAddrUses + regression test Function rewriteValueUsesToAddrUses() translates IRFieldExtract/IRGetElement/IRStore instructions that are affected by the change of the function parameter type from value to pointer. However, the implementation failed to consider the case where the index of IRGetElement() is IRFieldExtract() in which case the translated IRGetElementPtr() would be passed a pointer as the index. Add a second phase in this function that fixes this issue by adding a pointer load. Add also a minimal repro for this case as a regression test. Issue #9073 --- .../slang-ir-transform-params-to-constref.cpp | 53 +++++++++++++++++++ ...073-spirv-pointer-double-dereference.slang | 26 +++++++++ 2 files changed, 79 insertions(+) create mode 100644 tests/bugs/gh-9073-spirv-pointer-double-dereference.slang diff --git a/source/slang/slang-ir-transform-params-to-constref.cpp b/source/slang/slang-ir-transform-params-to-constref.cpp index 95ce31327f6..8355ecb32ab 100644 --- a/source/slang/slang-ir-transform-params-to-constref.cpp +++ b/source/slang/slang-ir-transform-params-to-constref.cpp @@ -43,10 +43,38 @@ struct TransformParamsToConstRefContext void rewriteValueUsesToAddrUses(IRInst* newAddrInst) { + // The overall strategy here is as follows: + // + // Phase 1 + // - Add IRLoad in front of the uses of newAddrInst, since the users need the + // value of the address. Except: + // - In case the user is IRFieldExtract/IRGetElement, rewrite those as + // IRFieldAddress and IRGetElementPtr and consume the base pointer as is + // - Scan transitively the uses of the transformed ops, and add IRLoad + // in front of them as well. + // + // Phase 2 (post-fixup) + // - Check all translated IRGetElementPtr instructions. If the index is + // a newly translated IRGetElementPtr or IRFieldAddress, add an IRLoad + // to fetch the value of the pointer. This load was omitted in Phase 1 + // due to the exception. + // - Note that the post-fixup phase avoids problems with rewrite + // ordering, so we don't try to fuse this in Phase 1 + // + // In addition, IRStore instructions are attempted to also optimize. See + // the related comment below. + HashSet workListSet; workListSet.add(newAddrInst); List workList; workList.add(newAddrInst); + + // List of IR instructions that may require a fixup after the rewrite + // pass. In particular, the index of IRGetElementPtr might use another + // IRGetElementPtr or IRFieldAddress, in which case we need to load the + // value of the pointer + List postFixList; + auto _addToWorkList = [&](IRInst* inst) { if (workListSet.add(inst)) @@ -90,6 +118,7 @@ struct TransformParamsToConstRefContext getElement->getIndex()); getElement->replaceUsesWith(elemAddr); _addToWorkList(elemAddr); + postFixList.add(elemAddr); return; } case kIROp_Store: @@ -117,6 +146,30 @@ struct TransformParamsToConstRefContext use->set(loadInst); }); } + + for (IRInst *inst : postFixList) + { + auto getElementPtr = as(inst); + IRInst *indexInst = getElementPtr->getOperand(1); + + // In the postfix phase, we'll only consider index instructions if + // they were transformed previously by this function. This is an + // extra precaution to avoid hiding bugs by other passes, should + // they also pass pointers directly as the index. + if (workListSet.contains(indexInst)) + { + switch (indexInst->getOp()) + { + case kIROp_FieldAddress: + case kIROp_GetElementPtr: + // post-fix needed + builder.setInsertBefore(getElementPtr); + auto loadInst = builder.emitLoad(indexInst); + getElementPtr->setOperand(1, loadInst); + break; + } + } + } } void rewriteParamUseSitesToSupportConstRefUsage(HashSet& updatedParams) diff --git a/tests/bugs/gh-9073-spirv-pointer-double-dereference.slang b/tests/bugs/gh-9073-spirv-pointer-double-dereference.slang new file mode 100644 index 00000000000..c1c5d0ee3db --- /dev/null +++ b/tests/bugs/gh-9073-spirv-pointer-double-dereference.slang @@ -0,0 +1,26 @@ +//TEST:SIMPLE(filecheck=POSITIVE): -entry main -stage closesthit -target spirv + +// This is a minimal repro for issue 9073 + +struct Payload +{ + uint volumeStackIndex; + int volumeStack[2]; + + int GetVolume() + { + return volumeStack[volumeStackIndex]; + } +} + +RWStructuredBuffer outputBuffer; + +[shader("closesthit")] +void main(inout Payload payload) +{ +// POSITIVE-NOT: {{(error|warning).*}}: +// POSITIVE: result code = 0 +// POSITIVE-NOT: {{(error|warning).*}}: + + outputBuffer[0] = payload.GetVolume(); +} From f193dead33a3ba04ead830bfe6339b3fc950420d Mon Sep 17 00:00:00 2001 From: slangbot <186143334+slangbot@users.noreply.github.com> Date: Mon, 24 Nov 2025 19:22:53 +0000 Subject: [PATCH 2/6] format code --- .../slang-ir-transform-params-to-constref.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/source/slang/slang-ir-transform-params-to-constref.cpp b/source/slang/slang-ir-transform-params-to-constref.cpp index 8355ecb32ab..433acfc7b1e 100644 --- a/source/slang/slang-ir-transform-params-to-constref.cpp +++ b/source/slang/slang-ir-transform-params-to-constref.cpp @@ -147,10 +147,10 @@ struct TransformParamsToConstRefContext }); } - for (IRInst *inst : postFixList) + for (IRInst* inst : postFixList) { auto getElementPtr = as(inst); - IRInst *indexInst = getElementPtr->getOperand(1); + IRInst* indexInst = getElementPtr->getOperand(1); // In the postfix phase, we'll only consider index instructions if // they were transformed previously by this function. This is an @@ -160,13 +160,13 @@ struct TransformParamsToConstRefContext { switch (indexInst->getOp()) { - case kIROp_FieldAddress: - case kIROp_GetElementPtr: - // post-fix needed - builder.setInsertBefore(getElementPtr); - auto loadInst = builder.emitLoad(indexInst); - getElementPtr->setOperand(1, loadInst); - break; + case kIROp_FieldAddress: + case kIROp_GetElementPtr: + // post-fix needed + builder.setInsertBefore(getElementPtr); + auto loadInst = builder.emitLoad(indexInst); + getElementPtr->setOperand(1, loadInst); + break; } } } From fec541658955ac3e907a881f5b22560079496d25 Mon Sep 17 00:00:00 2001 From: "Sami Kiminki (NVIDIA)" <235843927+skiminki-nv@users.noreply.github.com> Date: Tue, 25 Nov 2025 17:13:58 +0200 Subject: [PATCH 3/6] Add a test case for GetElement(FieldExtract(...), GetElement(...)) Update the commentary in transformation code --- .../slang-ir-transform-params-to-constref.cpp | 24 +++++------ ...073-spirv-pointer-double-dereference.slang | 41 +++++++++++++++++++ 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/source/slang/slang-ir-transform-params-to-constref.cpp b/source/slang/slang-ir-transform-params-to-constref.cpp index 433acfc7b1e..c61b804c2f1 100644 --- a/source/slang/slang-ir-transform-params-to-constref.cpp +++ b/source/slang/slang-ir-transform-params-to-constref.cpp @@ -53,16 +53,16 @@ struct TransformParamsToConstRefContext // - Scan transitively the uses of the transformed ops, and add IRLoad // in front of them as well. // - // Phase 2 (post-fixup) + // Phase 2 // - Check all translated IRGetElementPtr instructions. If the index is // a newly translated IRGetElementPtr or IRFieldAddress, add an IRLoad - // to fetch the value of the pointer. This load was omitted in Phase 1 - // due to the exception. - // - Note that the post-fixup phase avoids problems with rewrite - // ordering, so we don't try to fuse this in Phase 1 + // to fetch the value of the pointer. This insertion of load may have + // been omitted in Phase 1 due to the exception. + // - Note that doing a separate phase avoids problems with IR + // op rewrite ordering, since all rewrites are already done in Phase 1 // - // In addition, IRStore instructions are attempted to also optimize. See - // the related comment below. + // In addition, IRStore instructions are also attempted to be + // optimized. See the related comment below. HashSet workListSet; workListSet.add(newAddrInst); @@ -152,17 +152,17 @@ struct TransformParamsToConstRefContext auto getElementPtr = as(inst); IRInst* indexInst = getElementPtr->getOperand(1); - // In the postfix phase, we'll only consider index instructions if - // they were transformed previously by this function. This is an - // extra precaution to avoid hiding bugs by other passes, should - // they also pass pointers directly as the index. + // We'll only consider index instructions if they were transformed + // previously by this function. This is an extra precaution to avoid + // hiding bugs by other passes, should they also pass pointers + // directly as the IRGetElementPtr index. if (workListSet.contains(indexInst)) { switch (indexInst->getOp()) { case kIROp_FieldAddress: case kIROp_GetElementPtr: - // post-fix needed + // additional IRLoad() needed builder.setInsertBefore(getElementPtr); auto loadInst = builder.emitLoad(indexInst); getElementPtr->setOperand(1, loadInst); diff --git a/tests/bugs/gh-9073-spirv-pointer-double-dereference.slang b/tests/bugs/gh-9073-spirv-pointer-double-dereference.slang index c1c5d0ee3db..6d03c043d52 100644 --- a/tests/bugs/gh-9073-spirv-pointer-double-dereference.slang +++ b/tests/bugs/gh-9073-spirv-pointer-double-dereference.slang @@ -9,8 +9,47 @@ struct Payload int GetVolume() { + // This function exercises the following transformation in + // transformParamsToConstRef(): + // + // IRGetElement( + // IRFieldExtract(...), + // IRFieldExtract(...)) + // + // => + // + // IRGetElementPtr( + // IRFieldAddress(...), + // IRLoad(IRFieldAddress(...))) + // + // That is, that IRLoad() is correctly injected to + // IRGetElementPtr() index when IRFieldExtract() is + // transformed to IRFieldAddress() + return volumeStack[volumeStackIndex]; } + + int GetVolume2() + { + // This function exercises the following transformation in + // transformParamsToConstRef(): + // + // IRGetElement( + // IRFieldExtract(...), + // IRGetElement(...)) + // + // => + // + // IRGetElementPtr( + // IRFieldAddress(...), + // IRLoad(IRGetElementPtr(...))) + // + // That is, that IRLoad() is correctly injected to + // IRGetElementPtr() index when IRGetElement() is + // transformed to IRGetElementPtr() + + return volumeStack[volumeStack[volumeStackIndex]]; + } } RWStructuredBuffer outputBuffer; @@ -23,4 +62,6 @@ void main(inout Payload payload) // POSITIVE-NOT: {{(error|warning).*}}: outputBuffer[0] = payload.GetVolume(); + + outputBuffer[1] = payload.GetVolume2(); } From 421d1a488e6f75f2a95f2266a65cb8beee1d4a39 Mon Sep 17 00:00:00 2001 From: "Sami Kiminki (NVIDIA)" <235843927+skiminki-nv@users.noreply.github.com> Date: Thu, 27 Nov 2025 23:53:53 +0200 Subject: [PATCH 4/6] Rewrite rewriteValueUsesToAddrUses() --- .../slang-ir-transform-params-to-constref.cpp | 185 +++++++++--------- 1 file changed, 90 insertions(+), 95 deletions(-) diff --git a/source/slang/slang-ir-transform-params-to-constref.cpp b/source/slang/slang-ir-transform-params-to-constref.cpp index c61b804c2f1..930beec3bcf 100644 --- a/source/slang/slang-ir-transform-params-to-constref.cpp +++ b/source/slang/slang-ir-transform-params-to-constref.cpp @@ -45,82 +45,93 @@ struct TransformParamsToConstRefContext { // The overall strategy here is as follows: // - // Phase 1 - // - Add IRLoad in front of the uses of newAddrInst, since the users need the - // value of the address. Except: - // - In case the user is IRFieldExtract/IRGetElement, rewrite those as - // IRFieldAddress and IRGetElementPtr and consume the base pointer as is - // - Scan transitively the uses of the transformed ops, and add IRLoad - // in front of them as well. - // - // Phase 2 - // - Check all translated IRGetElementPtr instructions. If the index is - // a newly translated IRGetElementPtr or IRFieldAddress, add an IRLoad - // to fetch the value of the pointer. This insertion of load may have - // been omitted in Phase 1 due to the exception. - // - Note that doing a separate phase avoids problems with IR - // op rewrite ordering, since all rewrites are already done in Phase 1 - // - // In addition, IRStore instructions are also attempted to be - // optimized. See the related comment below. - - HashSet workListSet; - workListSet.add(newAddrInst); - List workList; - workList.add(newAddrInst); + // - First, insert IRLoad() in front of all uses of newAddrInst. This + // is a value parameter turned into pointer to value. Add all IRLoad() + // instructions in the working set + // - Then, for every inserted IRLoad() instruction, search for + // IRFieldExtract(IRLoad(ptr), ...) and IRGetElement(IRLoad(ptr), ...) patterns, + // and transform these to IRLoad(IRFieldAddress(ptr, ...)) and + // IRLoad(IRGetElementPtr(ptr, ...)), and insert the new IRLoad() + // instructions in the working set + // - Remove also stores to write-once temporary variables that are + // immediately passed into a constref location in a call (see below) + // - If all uses of the inserted IRLoad() were translated, remove the + // IRLoad() to keep this pass clean + + List workList; + + traverseUses( + newAddrInst, + [&](IRUse* use) + { + auto user = use->getUser(); + builder.setInsertBefore(user); + IRLoad* loadInst = as(builder.emitLoad(newAddrInst)); + use->set(loadInst); - // List of IR instructions that may require a fixup after the rewrite - // pass. In particular, the index of IRGetElementPtr might use another - // IRGetElementPtr or IRFieldAddress, in which case we need to load the - // value of the pointer - List postFixList; + workList.add(loadInst); + }); - auto _addToWorkList = [&](IRInst* inst) - { - if (workListSet.add(inst)) - workList.add(inst); - }; for (Index i = 0; i < workList.getCount(); i++) { - auto inst = workList[i]; + IRLoad* loadInst = workList[i]; + bool allUsesTranslated = true; + traverseUses( - inst, + loadInst, [&](IRUse* use) { - auto user = use->getUser(); - if (workListSet.contains(user)) - return; - switch (user->getOp()) + IRInst* userInst = use->getUser(); + bool useTranslated = false; + + switch (userInst->getOp()) { case kIROp_FieldExtract: { - // Transform the IRFieldExtract into a IRFieldAddress - if (!isUseBaseAddrOperand(use, user)) - break; - auto fieldExtract = as(user); - builder.setInsertBefore(fieldExtract); - auto fieldAddr = builder.emitFieldAddress( - fieldExtract->getBase(), - fieldExtract->getField()); - fieldExtract->replaceUsesWith(fieldAddr); - _addToWorkList(fieldAddr); - return; + if (isUseBaseAddrOperand(use, userInst)) + { + // Transform IRFieldExtract(IRLoad(ptr), x) + // ==> + // IRLoad(IRFieldAddr(ptr), x) + + auto fieldExtract = as(userInst); + builder.setInsertBefore(fieldExtract); + auto fieldAddr = builder.emitFieldAddress( + loadInst->getPtr(), + fieldExtract->getField()); + auto loadFieldAddr = as(builder.emitLoad(fieldAddr)); + fieldExtract->replaceUsesWith(loadFieldAddr); + fieldExtract->removeAndDeallocate(); + + workList.add(loadFieldAddr); + useTranslated = true; + } + break; } + case kIROp_GetElement: { - // Transform the IRGetElement into a IRGetElementPtr - if (!isUseBaseAddrOperand(use, user)) - break; - auto getElement = as(user); - builder.setInsertBefore(getElement); - auto elemAddr = builder.emitElementAddress( - getElement->getBase(), - getElement->getIndex()); - getElement->replaceUsesWith(elemAddr); - _addToWorkList(elemAddr); - postFixList.add(elemAddr); - return; + if (isUseBaseAddrOperand(use, userInst)) + { + // Transform IRGetElement(IRLoad(ptr), x) + // ==> + // IRLoad(IRGetElementPtr(ptr), x) + + auto getElement = as(userInst); + builder.setInsertBefore(getElement); + auto getElementPtr = builder.emitElementAddress( + loadInst->getPtr(), + getElement->getIndex()); + auto loadElementPtr = as(builder.emitLoad(getElementPtr)); + getElement->replaceUsesWith(loadElementPtr); + getElement->removeAndDeallocate(); + + workList.add(loadElementPtr); + useTranslated = true; + } + break; } + case kIROp_Store: { // If the current value is being stored into a write-once temp var that @@ -128,47 +139,31 @@ struct TransformParamsToConstRefContext // rid of the temp var and replace it with `inst` directly. // (such temp var can be introduced during `updateCallSites` when we // were processing the callee.) + + // Transform IRStore(ptr, load(x)) where ptr has attribute + // TempCallArgImmutableVarDecoration + // IRCall(func, ptr, ...) // - auto dest = as(user)->getPtr(); - if (dest->findDecorationImpl(kIROp_TempCallArgImmutableVarDecoration)) + // ==> IRCall(func, x, ...) + + auto storeDest = as(userInst)->getPtr(); + if (storeDest->findDecorationImpl( + kIROp_TempCallArgImmutableVarDecoration)) { - user->removeAndDeallocate(); - dest->replaceUsesWith(inst); - dest->removeAndDeallocate(); - return; + storeDest->replaceUsesWith(loadInst->getPtr()); + userInst->removeAndDeallocate(); + storeDest->removeAndDeallocate(); + useTranslated = true; } break; } } - // Insert a load before the user and replace the user with the load - builder.setInsertBefore(user); - auto loadInst = builder.emitLoad(inst); - use->set(loadInst); + + allUsesTranslated = allUsesTranslated && useTranslated; }); - } - for (IRInst* inst : postFixList) - { - auto getElementPtr = as(inst); - IRInst* indexInst = getElementPtr->getOperand(1); - - // We'll only consider index instructions if they were transformed - // previously by this function. This is an extra precaution to avoid - // hiding bugs by other passes, should they also pass pointers - // directly as the IRGetElementPtr index. - if (workListSet.contains(indexInst)) - { - switch (indexInst->getOp()) - { - case kIROp_FieldAddress: - case kIROp_GetElementPtr: - // additional IRLoad() needed - builder.setInsertBefore(getElementPtr); - auto loadInst = builder.emitLoad(indexInst); - getElementPtr->setOperand(1, loadInst); - break; - } - } + if (allUsesTranslated) + loadInst->removeAndDeallocate(); } } From c479318e969facb4ed37865af7b2773a0ce406e6 Mon Sep 17 00:00:00 2001 From: "Sami Kiminki (NVIDIA)" <235843927+skiminki-nv@users.noreply.github.com> Date: Wed, 3 Dec 2025 16:31:45 +0200 Subject: [PATCH 5/6] Clarify rewriteValueUsesToAddrUses() a bit --- .../slang-ir-transform-params-to-constref.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/source/slang/slang-ir-transform-params-to-constref.cpp b/source/slang/slang-ir-transform-params-to-constref.cpp index 930beec3bcf..ae9b8f8fb04 100644 --- a/source/slang/slang-ir-transform-params-to-constref.cpp +++ b/source/slang/slang-ir-transform-params-to-constref.cpp @@ -140,13 +140,18 @@ struct TransformParamsToConstRefContext // (such temp var can be introduced during `updateCallSites` when we // were processing the callee.) - // Transform IRStore(ptr, load(x)) where ptr has attribute - // TempCallArgImmutableVarDecoration - // IRCall(func, ptr, ...) - // - // ==> IRCall(func, x, ...) + // Transform IRStore(storeDest, load(ptr)) where storeDest has attribute + // TempCallArgImmutableVarDecoration + // IRInst(storeDest) + // ==> + // IRInst(ptr) + + auto storeInst = as(userInst); + auto storeDest = storeInst->getPtr(); + + // double-check that the value is actually the load instruction + assert(storeInst->getVal() == loadInst); - auto storeDest = as(userInst)->getPtr(); if (storeDest->findDecorationImpl( kIROp_TempCallArgImmutableVarDecoration)) { From a0e7eadea04bd5e751d767ec429b5894f38db24a Mon Sep 17 00:00:00 2001 From: "Sami Kiminki (NVIDIA)" <235843927+skiminki-nv@users.noreply.github.com> Date: Thu, 4 Dec 2025 12:06:45 +0200 Subject: [PATCH 6/6] Ensure that the we're matching against the IRStore val operand (not ptr) --- source/slang/slang-ir-transform-params-to-constref.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source/slang/slang-ir-transform-params-to-constref.cpp b/source/slang/slang-ir-transform-params-to-constref.cpp index ae9b8f8fb04..b427504e3f6 100644 --- a/source/slang/slang-ir-transform-params-to-constref.cpp +++ b/source/slang/slang-ir-transform-params-to-constref.cpp @@ -149,10 +149,8 @@ struct TransformParamsToConstRefContext auto storeInst = as(userInst); auto storeDest = storeInst->getPtr(); - // double-check that the value is actually the load instruction - assert(storeInst->getVal() == loadInst); - - if (storeDest->findDecorationImpl( + if (storeInst->getValUse() == use && + storeDest->findDecorationImpl( kIROp_TempCallArgImmutableVarDecoration)) { storeDest->replaceUsesWith(loadInst->getPtr());