Skip to content

Add internal-use __addressOf instrinsic to replace &#10216

Closed
aidanfnv wants to merge 2 commits intoshader-slang:masterfrom
aidanfnv:fix/internal-address-of
Closed

Add internal-use __addressOf instrinsic to replace &#10216
aidanfnv wants to merge 2 commits intoshader-slang:masterfrom
aidanfnv:fix/internal-address-of

Conversation

@aidanfnv
Copy link
Copy Markdown
Contributor

@aidanfnv aidanfnv commented Feb 25, 2026

The prefix & operator for taking addresses is being deprecated, but internal Slang code and tests need an equivalent mechanism.

This change adds __addressOf, an internal-use intrinsic declared in core.meta.slang with the same __intrinsic_op(0) behavior as &. It has a new KnownBuiltinDeclName::InternalAddressOf attribute, and the existing immutable-object check in CheckInvokeExprWithCheckedOperands is extended to also match InternalAddressOf, so that error 30079 still fires when passing a read-only value, like &. This change also migrates all internal and test usages of & for address-of to __addressOf.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Feb 25, 2026

📝 Walkthrough

Walkthrough

This pull request introduces a new generic intrinsic operation __addressOf that provides an alternative address-taking mechanism alongside the existing address-of operator. The change includes the intrinsic definition, corresponding enum member, compiler checking logic updates, and systematic replacement of address-of usage across test files and a standard library module.

Changes

Cohort / File(s) Summary
Core Language Infrastructure
source/slang/core.meta.slang, source/slang/slang-ast-support-types.h
Adds new __addressOf intrinsic operation declaration with KnownBuiltin(InternalAddressOf) attribute and require(cpp_cuda_spirv_llvm) constraint. Introduces corresponding InternalAddressOf enum member to KnownBuiltinDeclName.
Compiler Type Checking
source/slang/slang-check-expr.cpp
Extends constant-pointer diagnostic logic to handle both OperatorAddressOf and InternalAddressOf builtin names as unable to accept constant pointers.
Standard Library
source/standard-modules/neural/bindless-storage.slang
Updates BindlessAddress<T>.atomicAdd for vector<T, 2> on CUDA to use __addressOf(handle[baseIndex + index]) instead of address-of operator.
Test Files
tests/bugs/gh-3601.slang, tests/bugs/gh-7499.slang, tests/bugs/gh-8659.slang
Replaces address-of operator with __addressOf(...) intrinsic in various pointer-related test scenarios.
CPU Program Tests
tests/cpu-program/gfx-smoke.slang, tests/cpu-program/pointer-basics.slang, tests/cpu-program/pointer-deref.slang
Systematically replaces address-of operator usage with __addressOf(...) intrinsic across device creation, pointer initialization, and descriptor passing patterns.
Diagnostic & Feature Tests
tests/diagnostics/invalid-constant-pointer-taking.slang, tests/language-feature/bitfield/*.slang, tests/language-feature/pointer/coherent-load-store-physical-storage-buffer.slang, tests/llvm/inline-llvm-ir.slang, tests/spirv/pointer.slang
Replaces address-of operator with __addressOf(...) intrinsic for struct addressing, bitfield representation, coherent loads, inline LLVM IR, and SPIR-V pointer operations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A rabbit hops through pointers bright,
With __addressOf now in sight,
No more just & to take our way,
New intrinsics guide the day! 🐰✨
Address-taking, clean and right.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: introducing a new internal __addressOf intrinsic to replace the deprecated & operator.
Description check ✅ Passed The description is directly related to the changeset, explaining the motivation for the new intrinsic and detailing the implementation approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@aidanfnv aidanfnv added the pr: non-breaking PRs without breaking changes label Feb 25, 2026
@aidanfnv aidanfnv changed the title Add internal-use __addressOf instrinsic Add internal-use __addressOf instrinsic to replace & Feb 25, 2026
@aidanfnv aidanfnv marked this pull request as ready for review February 25, 2026 23:33
@aidanfnv aidanfnv requested a review from a team as a code owner February 25, 2026 23:33
@aidanfnv aidanfnv requested review from bmillsNV and Copilot and removed request for a team February 25, 2026 23:33
@aidanfnv aidanfnv removed the request for review from bmillsNV February 25, 2026 23:37
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new __addressOf intrinsic to replace the deprecated & (address-of) operator for internal Slang code and tests. The change maintains identical behavior to the & operator, including the immutable-object safety check (error 30079), while providing a clear internal-use alternative as the public & operator is being deprecated.

Changes:

  • Added __addressOf intrinsic declaration in core.meta.slang with matching semantics to & operator
  • Extended immutable-object validation to also check InternalAddressOf in addition to OperatorAddressOf
  • Migrated all internal code and test usages from & to __addressOf across 13 test files and 1 standard module file

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Show a summary per file
File Description
source/slang/core.meta.slang Declares new __addressOf intrinsic with __intrinsic_op(0) and KnownBuiltinDeclName::InternalAddressOf attribute
source/slang/slang-ast-support-types.h Adds InternalAddressOf to KnownBuiltinDeclName enum
source/slang/slang-check-expr.cpp Extends immutable-object check to validate InternalAddressOf alongside OperatorAddressOf
tests/spirv/pointer.slang Migrates &p.data to __addressOf(p.data)
tests/llvm/inline-llvm-ir.slang Migrates &a to __addressOf(a) in memset call
tests/language-feature/pointer/coherent-load-store-physical-storage-buffer.slang Migrates &secondPtrIn[1] to __addressOf(secondPtrIn[1])
tests/language-feature/bitfield/repr.slang Migrates &s to __addressOf(s) in cast expression
tests/language-feature/bitfield/repr-mixed.slang Migrates &m to __addressOf(m) in cast expression
tests/language-feature/bitfield/msvc-repr.slang Migrates &s to __addressOf(s) in cast expression
tests/diagnostics/invalid-constant-pointer-taking.slang Migrates test to verify error 30079 still fires with __addressOf on constant buffers
tests/cpu-program/pointer-deref.slang Migrates &rec to __addressOf(rec)
tests/cpu-program/pointer-basics.slang Migrates two uses of &value to __addressOf(value)
tests/cpu-program/gfx-smoke.slang Migrates 9 address-of operations to __addressOf in API calls
tests/bugs/gh-8659.slang Migrates &ptr[0] to __addressOf(ptr[0])
tests/bugs/gh-7499.slang Migrates &arr to __addressOf(arr) in function call
tests/bugs/gh-3601.slang Migrates &p.data to __addressOf(p.data)
source/standard-modules/neural/bindless-storage.slang Migrates &handle[baseIndex + index] to __addressOf(...) in CUDA atomic operation

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/diagnostics/invalid-constant-pointer-taking.slang (1)

9-17: ⚠️ Potential issue | 🟡 Minor

Clarify whether __addressOf on an RWStructuredBuffer element is intentionally unchecked.

The file-level comment (Line 9) reads: "We do not allow taking a pointer from a StructuredBuffer/RWStructuredBuffer."

After the migration:

  • __getAddress(mutable_float_buffer[...]) (Line 17) → error 31160
  • __addressOf(mutable_float_buffer[...]) (Line 14) → no error CHECK

Two interpretations:

  1. Intentional escape hatch__addressOf deliberately bypasses the __getAddress guard for mutable buffers, in which case the comment on Line 9 is now stale and should be narrowed to apply only to __getAddress.
  2. Accidental omission — the error 31160 CHECK for the mutable case was dropped during migration, introducing a diagnostic regression.

Please either add the missing // CHECK: ([[# @line+1]]): error ... directive on Line 13, or update the comment on Line 9 to document that __addressOf is permitted on RWStructuredBuffer elements.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/diagnostics/invalid-constant-pointer-taking.slang` around lines 9 - 17,
The test currently asserts an error for __getAddress but not for __addressOf on
elements of mutable_float_buffer; decide whether this is a regression or
intended behavior and update the test accordingly: if __addressOf should also be
disallowed, add a CHECK directive before the line that declares mutablePtr1
asserting the same diagnostic (error 31160) for
__addressOf(mutable_float_buffer[threadId.x]); otherwise, update the file-level
comment that currently says "We do not allow taking a pointer from a
StructuredBuffer/RWStructuredBuffer." to clarify that the restriction applies
only to __getAddress and that __addressOf is permitted, referencing the symbols
__addressOf, __getAddress, and mutable_float_buffer in the updated comment so
the intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tests/diagnostics/invalid-constant-pointer-taking.slang`:
- Around line 9-17: The test currently asserts an error for __getAddress but not
for __addressOf on elements of mutable_float_buffer; decide whether this is a
regression or intended behavior and update the test accordingly: if __addressOf
should also be disallowed, add a CHECK directive before the line that declares
mutablePtr1 asserting the same diagnostic (error 31160) for
__addressOf(mutable_float_buffer[threadId.x]); otherwise, update the file-level
comment that currently says "We do not allow taking a pointer from a
StructuredBuffer/RWStructuredBuffer." to clarify that the restriction applies
only to __getAddress and that __addressOf is permitted, referencing the symbols
__addressOf, __getAddress, and mutable_float_buffer in the updated comment so
the intent is clear.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between ed97591 and b654f2e.

📒 Files selected for processing (17)
  • source/slang/core.meta.slang
  • source/slang/slang-ast-support-types.h
  • source/slang/slang-check-expr.cpp
  • source/standard-modules/neural/bindless-storage.slang
  • tests/bugs/gh-3601.slang
  • tests/bugs/gh-7499.slang
  • tests/bugs/gh-8659.slang
  • tests/cpu-program/gfx-smoke.slang
  • tests/cpu-program/pointer-basics.slang
  • tests/cpu-program/pointer-deref.slang
  • tests/diagnostics/invalid-constant-pointer-taking.slang
  • tests/language-feature/bitfield/msvc-repr.slang
  • tests/language-feature/bitfield/repr-mixed.slang
  • tests/language-feature/bitfield/repr.slang
  • tests/language-feature/pointer/coherent-load-store-physical-storage-buffer.slang
  • tests/llvm/inline-llvm-ir.slang
  • tests/spirv/pointer.slang

@aidanfnv
Copy link
Copy Markdown
Contributor Author

Discussed with Yong on Discord that this should not be added and we should instead change __getAddress to not emit an error for CPU or CUDA, so that __getAddress can be used for all of the tests changed here.

@aidanfnv aidanfnv closed this Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: non-breaking PRs without breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants