Replace operator & usage with __getAddress#10280
Replace operator & usage with __getAddress#10280aidanfnv wants to merge 18 commits intoshader-slang:masterfrom
& usage with __getAddress#10280Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Knowledge base: Disabled due to 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds compound capability Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Frontend as Frontend (AST)
participant Lower as Lowering (IR)
participant IRMod as IRModule
participant Validator as validateGetAddressUsage
participant Emitter as Emitter/PassManager
participant Sink as DiagnosticSink
Frontend->>Lower: produce AddressOfExpr / __getAddress() nodes
Lower->>IRMod: lower to IR, attach GetAddressDecoration to IRVar
Emitter->>Validator: run validateGetAddressUsage(module, sink)
Validator->>IRMod: traverse IR, follow derived-pointer chains
Validator->>Sink: emit invalid-address-of diagnostic if function-local addresses stored
Emitter->>Emitter: continue remaining passes / emit target code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR modifies IR instruction definition files. Please review if you need to update the following constants in
These version numbers help ensure compatibility between different versions of compiled modules. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
aidanfnv
requested review from
bmillsNV and
Copilot
and removed request for
a team and
bmillsNV
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR migrates test and compiler behavior away from using the unary & address-of operator and toward __getAddress, adding IR plumbing to track/validate __getAddress usage across targets.
Changes:
- Updated multiple
.slangtests to replace address-of (&) with__getAddress(...). - Added a new IR decoration (
GetAddressDecoration) and an IR validation pass to reject__getAddressof function-local variables on non-CPU/CUDA targets. - Extended capability sets/documentation to include an LLVM-capable compound target and bumped IR module max supported version.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/spirv/pointer.slang | Uses __getAddress instead of & in SPIR-V pointer test. |
| tests/llvm/inline-llvm-ir.slang | Uses __getAddress for taking address of a local in LLVM test. |
| tests/language-feature/pointer/coherent-load-store-physical-storage-buffer.slang | Replaces & with __getAddress for coherent load/store pointer formation. |
| tests/language-feature/bitfield/repr.slang | Replaces & with __getAddress in bitfield representation test. |
| tests/language-feature/bitfield/repr-mixed.slang | Same address-of migration in mixed packing test. |
| tests/language-feature/bitfield/msvc-repr.slang | Same address-of migration in MSVC packing test. |
| tests/cpu-program/pointer-deref.slang | Replaces & with __getAddress in CPU pointer deref test. |
| tests/cpu-program/pointer-basics.slang | Replaces & with __getAddress in pointer casting/basic ops test. |
| tests/cpu-program/gfx-smoke.slang | Replaces & with __getAddress for passing struct addresses into gfx APIs. |
| tests/bugs/gh-8659.slang | Replaces & with __getAddress in bug regression test. |
| tests/bugs/gh-7499.slang | Replaces & with __getAddress in bug regression test. |
| tests/bugs/gh-3601.slang | Replaces & with __getAddress in bug regression test. |
| source/slang/slang-lower-to-ir.cpp | Marks lowered __getAddress temporaries with GetAddressDecoration. |
| source/slang/slang-ir.h | Bumps max supported IR module version (7 → 8). |
| source/slang/slang-ir-validate.h | Declares validateGetAddressUsage validation pass. |
| source/slang/slang-ir-validate.cpp | Implements validateGetAddressUsage and helper logic. |
| source/slang/slang-ir-insts.lua | Adds GetAddressDecoration IR decoration opcode. |
| source/slang/slang-ir-insts-stable-names.lua | Adds stable name mapping for the new decoration. |
| source/slang/slang-emit.cpp | Runs validateGetAddressUsage for non-CPU/non-CUDA targets. |
| source/slang/slang-check-expr.cpp | Diagnoses calls to built-in address-of operator and adjusts __getAddress typing rules. |
| source/slang/slang-check-decl.cpp | Updates capability requirement for __getAddress to include LLVM compound capability. |
| source/slang/slang-capabilities.capdef | Adds cpp_cuda_metal_spirv_llvm compound capability alias. |
| docs/user-guide/a3-02-reference-capability-atoms.md | Documents the new compound capability alias. |
Comments suppressed due to low confidence (1)
source/slang/slang-check-expr.cpp:3020
- This new unconditional
Diagnostics::invalidAddressOffor invocations of the built-in address-of operator (KnownBuiltinDeclName::OperatorAddressOf) will report an error message that specifically mentions__getAddress, even when the user wrote&expr. It also appears likely to change expected diagnostic IDs in existing tests that still use&(e.g.,tests/diagnostics/invalid-constant-pointer-taking.slangcurrently checks for error 30079 on&constant_float_buffer[...]). Consider introducing a dedicated diagnostic for the&operator (with migration guidance), and/or updating the affected diagnostic tests to match the new behavior.
Index paramCount = funcType->getParamCount();
for (Index pp = 0; pp < paramCount; ++pp)
{
auto paramType = funcType->getParamTypeWithModeWrapper(pp);
Expr* argExpr = nullptr;
ParamDecl* paramDecl = nullptr;
if (pp < invoke->arguments.getCount())
{
argExpr = invoke->arguments[pp];
if (funcDeclBase)
paramDecl = funcDeclBase->getParameters()[pp];
}
compareMemoryQualifierOfParamToArgument(paramDecl, argExpr);
if (as<OutParamTypeBase>(paramType) || as<RefParamType>(paramType))
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-ir-insts-stable-names.lua`:
- Line 739: You added a new stable name entry "Decoration.GetAddressDecoration"
with ID 737 but did not bump the module version; update the constant
k_maxSupportedModuleVersion from 8 to 9 in the header where it is defined
(symbol k_maxSupportedModuleVersion) so the new instruction ID is covered by the
supported module version.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (23)
docs/user-guide/a3-02-reference-capability-atoms.mdsource/slang/slang-capabilities.capdefsource/slang/slang-check-decl.cppsource/slang/slang-check-expr.cppsource/slang/slang-emit.cppsource/slang/slang-ir-insts-stable-names.luasource/slang/slang-ir-insts.luasource/slang/slang-ir-validate.cppsource/slang/slang-ir-validate.hsource/slang/slang-ir.hsource/slang/slang-lower-to-ir.cpptests/bugs/gh-3601.slangtests/bugs/gh-7499.slangtests/bugs/gh-8659.slangtests/cpu-program/gfx-smoke.slangtests/cpu-program/pointer-basics.slangtests/cpu-program/pointer-deref.slangtests/language-feature/bitfield/msvc-repr.slangtests/language-feature/bitfield/repr-mixed.slangtests/language-feature/bitfield/repr.slangtests/language-feature/pointer/coherent-load-store-physical-storage-buffer.slangtests/llvm/inline-llvm-ir.slangtests/spirv/pointer.slang
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-ir-validate.cpp`:
- Around line 817-840: Add a new unit/IR diagnostic test that exercises
validateGetAddressUsageImpl's path for plain function-local scalar variables so
the Diagnostics::invalidAddressOf case is triggered; create a test that compiles
the provided compute shader snippet (with a local int and int* p =
__getAddress(local);) and asserts the error code (31160) is emitted. Locate the
test harness that runs stage=compute/-entry computeMain (matching other
address-of tests) and model the new test after the existing structured-buffer
and group-shared tests so it exercises validateGetAddressUsageImpl, referencing
the function name computeMain, the validation function
validateGetAddressUsageImpl, and the diagnostic symbol invalidAddressOf to
ensure the code path and diagnostic are covered. Ensure the test is
named/described consistently with other tests so it runs in the same suite and
fails if the diagnostic is not produced.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
source/slang/slang-ir-validate.cpp
| static void validateGetAddressUsageImpl(DiagnosticSink* sink, IRInst* inst) | ||
| { | ||
| if (inst->getOp() == kIROp_Var && inst->findDecoration<IRGetAddressDecoration>()) | ||
| { | ||
| for (auto use = inst->firstUse; use; use = use->nextUse) | ||
| { | ||
| auto user = use->getUser(); | ||
| if (user->getOp() != kIROp_Store) | ||
| continue; | ||
| auto store = as<IRStore>(user); | ||
| if (store->getPtr() != inst) | ||
| continue; | ||
| if (isFunctionLocalAddress(store->getVal())) | ||
| { | ||
| sink->diagnose(inst->sourceLoc, Diagnostics::invalidAddressOf); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (auto child : inst->getModifiableChildren()) | ||
| { | ||
| validateGetAddressUsageImpl(sink, child); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any existing tests that exercise the invalidAddressOf diagnostic for __getAddress
# Search for tests that use __getAddress with expected diagnostics
rg -l '__getAddress.*local|invalidAddressOf' --glob '*.slang' tests/ 2>/dev/null
# Search for diagnostic test files related to getAddress
fd -e slang -e hlsl . tests/diagnostics/ --exec grep -l -i 'getaddress\|getAddress' {} \; 2>/dev/null
# Check if error code 31160 (invalidAddressOf) is tested anywhere
rg '31160|invalidAddressOf' tests/ 2>/dev/null
# Search for files mentioning the validation function itself
rg 'validateGetAddressUsageImpl' . 2>/dev/nullRepository: shader-slang/slang
Length of output: 719
🏁 Script executed:
#!/bin/bash
# Check the content of the existing test files
echo "=== get-address-validation.slang ==="
cat tests/language-feature/pointer/get-address-validation.slang
echo ""
echo "=== invalid-constant-pointer-taking.slang ==="
cat tests/diagnostics/invalid-constant-pointer-taking.slangRepository: shader-slang/slang
Length of output: 3416
Add a diagnostic test for function-local scalar address validation.
Tests exist for taking addresses from structured buffers and group-shared memory, but there is no test verifying the invalidAddressOf diagnostic fires when attempting to take the address of a plain function-local scalar variable. This validation path should be covered:
//TEST:SIMPLE(filecheck=CHECK):-stage compute -entry computeMain -target spirv
[numthreads(1,1,1)]
void computeMain()
{
int local = 42;
// CHECK: error 31160
int* p = __getAddress(local);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/slang/slang-ir-validate.cpp` around lines 817 - 840, Add a new unit/IR
diagnostic test that exercises validateGetAddressUsageImpl's path for plain
function-local scalar variables so the Diagnostics::invalidAddressOf case is
triggered; create a test that compiles the provided compute shader snippet (with
a local int and int* p = __getAddress(local);) and asserts the error code
(31160) is emitted. Locate the test harness that runs stage=compute/-entry
computeMain (matching other address-of tests) and model the new test after the
existing structured-buffer and group-shared tests so it exercises
validateGetAddressUsageImpl, referencing the function name computeMain, the
validation function validateGetAddressUsageImpl, and the diagnostic symbol
invalidAddressOf to ensure the code path and diagnostic are covered. Ensure the
test is named/described consistently with other tests so it runs in the same
suite and fails if the diagnostic is not produced.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/slang/slang-emit.cpp (1)
1628-1631:⚠️ Potential issue | 🟠 MajorRun
validateGetAddressUsageearlier to avoid optimization-elided false negatives.At this stage, the IR has already gone through several simplification/DCE passes, so the decorated-var/store pattern this check relies on may already be rewritten away. Please move this pass closer to post-link/pre-optimization validation.
Suggested pass placement adjustment
@@ - if (!isCPUTarget(targetRequest) && !isCUDATarget(targetRequest)) - { - SLANG_PASS(validateGetAddressUsage, sink); - } + // Run early while AddressOf lowering artifacts are still intact. + if (!isCPUTarget(targetRequest) && !isCUDATarget(targetRequest)) + { + SLANG_PASS(validateGetAddressUsage, sink); + }@@ - if (!isCPUTarget(targetRequest) && !isCUDATarget(targetRequest)) - { - SLANG_PASS(validateGetAddressUsage, sink); - } + // (moved earlier in pipeline)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-emit.cpp` around lines 1628 - 1631, The validateGetAddressUsage check is being run too late (after simplification/DCE) causing false negatives; call validateGetAddressUsage(sink) earlier in the pipeline before the optimization passes that rewrite the decorated-var/store pattern — i.e., move the SLANG_PASS(validateGetAddressUsage, sink) invocation out of the current isCPUTarget/isCUDATarget gated block and into the post-link/pre-optimization validation stage (ensure the call still respects targetRequest checks via isCPUTarget/isCUDATarget if needed), updating the placement so validateGetAddressUsage sees the IR prior to DCE/simplification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/slang/slang-emit.cpp`:
- Around line 1628-1631: The validateGetAddressUsage check is being run too late
(after simplification/DCE) causing false negatives; call
validateGetAddressUsage(sink) earlier in the pipeline before the optimization
passes that rewrite the decorated-var/store pattern — i.e., move the
SLANG_PASS(validateGetAddressUsage, sink) invocation out of the current
isCPUTarget/isCUDATarget gated block and into the post-link/pre-optimization
validation stage (ensure the call still respects targetRequest checks via
isCPUTarget/isCUDATarget if needed), updating the placement so
validateGetAddressUsage sees the IR prior to DCE/simplification.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
docs/command-line-slangc-reference.mdsource/slang/slang-check-decl.cppsource/slang/slang-check-expr.cppsource/slang/slang-emit.cppsource/slang/slang-ir-validate.cppsource/slang/slang-lower-to-ir.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- source/slang/slang-check-expr.cpp
- source/slang/slang-check-decl.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/slang/slang-ir-validate.cpp (1)
831-844: Avoid duplicateInvalidAddressOfemissions per decorated var.If multiple invalid stores reference the same decorated
IRVar, this loop can report the same site repeatedly. Emitting once per decorated var keeps diagnostics clean.Proposed tweak
for (auto use = inst->firstUse; use; use = use->nextUse) { auto user = use->getUser(); if (user->getOp() != kIROp_Store) continue; auto store = as<IRStore>(user); if (store->getPtr() != inst) continue; if (isFunctionLocalAddress(store->getVal())) { sink->diagnose(Diagnostics::InvalidAddressOf{ .location = inst->sourceLoc, }); + break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-ir-validate.cpp` around lines 831 - 844, This loop can emit duplicate Diagnostics::InvalidAddressOf for the same decorated variable when multiple stores reference the same inst; fix by tracking which var/address has already been diagnosed (e.g., maintain a small hash/set of IRInst* or IRVar* called diagnosedAddrs) in the surrounding function, and before calling sink->diagnose(...) check if inst (or the related IRVar) is already in diagnosedAddrs; if not, insert it and emit the diagnostic—use the existing symbols inst, use, user, IRStore, store, isFunctionLocalAddress and sink->diagnose to locate and guard the diagnostic emission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/slang/slang-ir-validate.cpp`:
- Around line 831-844: This loop can emit duplicate
Diagnostics::InvalidAddressOf for the same decorated variable when multiple
stores reference the same inst; fix by tracking which var/address has already
been diagnosed (e.g., maintain a small hash/set of IRInst* or IRVar* called
diagnosedAddrs) in the surrounding function, and before calling
sink->diagnose(...) check if inst (or the related IRVar) is already in
diagnosedAddrs; if not, insert it and emit the diagnostic—use the existing
symbols inst, use, user, IRStore, store, isFunctionLocalAddress and
sink->diagnose to locate and guard the diagnostic emission.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
source/slang/slang-check-expr.cppsource/slang/slang-diagnostics.luasource/slang/slang-ir-validate.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- source/slang/slang-check-expr.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Summary
This PR replaces all internal uses of the & (address-of) operator with __getAddress() as a step toward deprecating & for pointer creation. The key changes are:
- Relaxes
__getAddresssemantics: Previously only worked for groupshared/physical-storage-buffer variables; now accepts all variable types, falling back toUserPointeraddress space. - New IR validation pass (
validateGetAddressUsage): Rejects__getAddresson function-local variables for GPU targets (non-CPU/CUDA), using a newGetAddressDecorationon lowered IR vars. - New capability atom:
cpp_cuda_metal_spirv_llvmto extend__getAddressto LLVM targets. - Test migration: All
&usages in slang-test files replaced with__getAddress.
Key Findings
-
Diagnostic message is stale (slang-diagnostics.lua): The error message still says
'__getAddress' only supports groupshared variables and members of groupshared/device memory.This is no longer accurate —__getAddressnow works for all variables at the semantic level, with the IR pass catching function-local cases on GPU targets. The two different uses of this diagnostic (semantic check vs IR validation) warrant either different messages or a more generic phrasing. -
LLVM target coverage in validation skip (slang-emit.cpp): The skip condition
!isCPUTarget && !isCUDATargetshould implicitly cover LLVM targets (sinceisCPUTargetusesisCpuLikeTargetwhich includes LLVM), but this relationship is non-obvious. Explicitly includingisCPUTargetViaLLVMwould improve clarity. -
Validation pass ordering (slang-ir-validate.cpp): The validation runs late in the pipeline after IR simplification has already run. Optimization may fold the store-to-decorated-var pattern the pass looks for, making the check a best-effort diagnostic rather than a guaranteed safety net.
Positive Aspects
- Clean separation of concerns: The semantic layer accepts the broader set of cases, and the IR validation pass acts as a target-specific safety net — aligning with Slang's philosophy of doing heavy validation/transforms in IR passes.
- Well-structured IR integration: Proper use of FIDDLE-generated decoration, stable name registration, and module version bump.
- Thorough test migration: All
&usages systematically replaced. - Good documentation: PR description clearly explains the rationale and the multi-PR deprecation plan.
Overall Assessment
The approach is sound and well-motivated. The three inline comments are suggestions/observations rather than blocking issues. The diagnostic message accuracy is the most actionable item — consider updating it in this PR or a follow-up. The validation pass ordering concern is an inherent limitation of the pattern-matching approach and is acceptable for a best-effort diagnostic.
| 31160, | ||
| "invalid __getAddress usage", | ||
| span { loc = "expr:Expr", message = "'__getAddress' only supports groupshared variables and members of groupshared/device memory." } | ||
| span { loc = "location", message = "'__getAddress' only supports groupshared variables and members of groupshared/device memory." } |
There was a problem hiding this comment.
The diagnostic message says '__getAddress' only supports groupshared variables and members of groupshared/device memory. but this is no longer accurate after this PR. Now __getAddress accepts all variable types at the semantic level; it's only the IR validation pass that rejects function-local addresses on GPU targets.
This message is now used in two contexts:
- Semantic check (
visitAddressOfExpr): fires for non-lvalue expressions that can't be addressed at all - IR validation (
validateGetAddressUsageImpl): fires for function-local vars on GPU targets
For case 2, the message should probably say something like '__getAddress' cannot take the address of a function-local variable on this target. Consider either splitting into two diagnostics or updating the message to be more generic (e.g., '__getAddress' cannot take the address of this expression.).
| } | ||
|
|
||
| if (!isCPUTarget(targetRequest) && !isCUDATarget(targetRequest)) | ||
| { |
There was a problem hiding this comment.
The validation skip uses !isCPUTarget(targetRequest) && !isCUDATarget(targetRequest). The LLVM target (e.g., HostLLVMIR) is a CPU-like target with flat memory that should also skip validation.
I believe isCPUTarget() does cover LLVM targets (via ArtifactDescUtil::isCpuLikeTarget), so this should work correctly. However, the relationship is implicit and non-obvious — elsewhere in the codebase doesTargetSupportUnrestrictedPointers explicitly includes isCPUTargetViaLLVM(req) as a separate check. Consider adding !isCPUTargetViaLLVM(targetRequest) here as well for clarity and robustness, since the new cpp_cuda_metal_spirv_llvm capability explicitly enables __getAddress on LLVM targets.
There was a problem hiding this comment.
The isCPUTarget() check is correct in this context. isCPUTargetViaLLVM() only exists due to a potential future direction where LLVM could be used for some GPU targets as well and it's only supposed to be used when doing something specific to the combination of targeting CPUs with LLVM.
| for (auto use = inst->firstUse; use; use = use->nextUse) | ||
| { | ||
| auto user = use->getUser(); | ||
| if (user->getOp() != kIROp_Store) | ||
| continue; | ||
| auto store = as<IRStore>(user); | ||
| if (store->getPtr() != inst) | ||
| continue; | ||
| if (isFunctionLocalAddress(store->getVal())) | ||
| { | ||
| sink->diagnose(Diagnostics::InvalidAddressOf{ | ||
| .location = inst->sourceLoc, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (auto child : inst->getModifiableChildren()) | ||
| { | ||
| validateGetAddressUsageImpl(sink, child); | ||
| } | ||
| } | ||
|
|
||
| void validateGetAddressUsage(IRModule* module, DiagnosticSink* sink) | ||
| { | ||
| validateGetAddressUsageImpl(sink, module->getModuleInst()); |
There was a problem hiding this comment.
The validation pattern relies on finding kIROp_Store to the GetAddressDecoration-decorated var. However, IR simplification passes (which run much earlier in the pipeline — around line 1165 vs the validation placement at ~1627) may fold the store-load pair, eliminating the store and DCE-ing the var. In that case the function-local address flows through the IR undetected by this validation.
This is likely acceptable as a best-effort diagnostic (the invalid code would still fail during codegen/SPIRV-validation), but worth noting. You might consider placing this validation earlier (e.g. right after lowering, before simplification), or alternatively walking all uses of kIROp_Var with AddressSpace::Function to find any pointer-to-pointer escape paths rather than relying on the decoration.
|
|
||
| // Walk through derived-pointer ops to find the root allocation, | ||
| // then check if it lives in function-local memory. | ||
| static bool isFunctionLocalAddress(IRInst* val) |
There was a problem hiding this comment.
can call getRootAddr() instead.
csyonghe
left a comment
csyonghe
left a comment
There was a problem hiding this comment.
tmp = kIROp_AssumeAddress(addr);
The checking behavior is:
- When emitting code for cuda/cpp, no validation needed.
- Immediately after linkIr, When emitting code for other targets,
validate that getRootAddr(addr) is in
UserPointer address space, and if not emit an error.
After running the validation pass, immediately replace assumeAddr(x) with x.
so none of the backend IR passes need to worry about this new opcode.
The decision has been made to deprecate the use of
&to get pointers, and replace any internal use of it with__getAddress.Currently,
__getAddressis more restrictive than&. it only produces typed pointers for groupshared and physical-storage-buffer variables, so it can't serve as a drop-in replacement. This PR relaxes__getAddressto fall back toUserPointeraddress space for all remaining variable types (function-locals, device-memory, etc.), making it functionally equivalent to&.To preserve safety on GPU targets where function-local pointers are not representable, a new IR validation pass (
validateGetAddressUsage) rejects__getAddresson function-local variables before codegen. The pass uses a newGetAddressDecorationon lowered IR vars to identify address-of sites, then walks derived-pointer ops to find the root allocation and check its address space. CPU and CUDA targets skip this validation since they have flat memory.The
__getAddresscapability is also extended to the LLVM target via a newcpp_cuda_metal_spirv_llvmcompound capability atom.All existing slang-test uses of
&are replaced with__getAddress.Operator
&for taking addresses will be deprecated in a follow-up PR. It cannot be deprecated in this PR because SlangPy uses&, and SlangPy needs to be updated to use__getAddressfirst, otherwise the deprecation would break SlangPy's CI before the fix can land.