[codex] add fixed-size out array returns#341
Open
isVoid wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an out_array_return intent to return fixed-size C++ output arrays as UniTuple values: intent/def parsing and factory, intent normalization and dtype resolution, call-convention LLVM lowering to allocate/load fixed native buffers, binding/renderer refactors to compute return types and shim args, static-schema/docs updates, utils canonicalization, and tests. Changesout_array_return Intent Feature
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
numbast/tests/test_callconv.py (1)
125-183: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-indirect
out_array_returnlowering path.Current tests only validate
shim_arg_indirect=True. Please add one case withshim_arg_indirect=Falseto lock in the aggregate array alloca/GEP branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@numbast/tests/test_callconv.py` around lines 125 - 183, Add a new test that mirrors the existing out_array_return tests but sets OutArrayReturnSpec(shim_arg_indirect=False) to exercise the non-indirect lowering path; create an IntentPlan with ArgIntent.out_array_return, an appropriate return_type (e.g. UniTuple(dtype, length)), call _lower_callconv_to_ir with that plan, and assert the IR contains the aggregate-style alloca and GEP patterns (e.g. an "alloca [N x <elem>]" or similar aggregate alloca and a "getelementptr" into the aggregate) to lock in the aggregate array alloca/GEP branch rather than the indirect pointer path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@numbast/src/numbast/callconv.py`:
- Around line 24-33: The current _get_out_array_return_specs returns all-None
when out_array_return_specs is missing, which can mask cases where an IntentPlan
contains an out_array_return intent; change it to fail fast: in
_get_out_array_return_specs, if out_array_return_specs is falsy, scan
plan.intents for any intent that represents the out_array_return (match the
enum/constant used in your code, e.g., Intent.out_array_return or intent.name ==
"out_array_return"); if any such intent exists, raise a ValueError indicating a
missing out_array_return spec, otherwise return the tuple of Nones as before;
keep the existing length check (len(specs) != len(plan.intents)) and final
return tuple(specs).
In `@numbast/src/numbast/intent_utils.py`:
- Around line 28-35: The resolve_out_array_dtype function currently accepts
results from to_numba_type that can be numba.undefined (the sentinel for unknown
types) because it only filters nbtypes.Opaque; update resolve_out_array_dtype to
treat numba.undefined as an invalid resolution: after calling
to_numba_type(dtype) check if resolved is numba.undefined or is an instance of
nbtypes.Opaque and, if so, raise the same ValueError; otherwise return the
resolved nbtypes.Type. This refers to resolve_out_array_dtype, to_numba_type,
nbtypes.Opaque and the numba.undefined sentinel.
In `@numbast/src/numbast/intent.py`:
- Around line 70-72: The helper _is_pointer_type currently only checks for "*"
in getattr(ast_type, "unqualified_non_ref_type_name"); extend it to also
recognize native array spellings like "T[N]" or "T[]" by matching brackets in
the type name (e.g., check for "[" and "]" or use a regex like
r"\[\s*\d*\s*\]"); update _is_pointer_type accordingly so the out_array_return
flow (and the logic referenced around out_array_return) treats fixed-size/native
arrays as pointer-like and is accepted where "*" is currently detected.
In `@numbast/src/numbast/static/function.py`:
- Around line 67-122: The out-array string-rendering helpers (_tuple_literal,
_out_array_specs_for_plan, _out_return_type_str, _compose_return_type_str,
_render_out_array_specs) are duplicated in struct.py; extract them into a single
shared module (e.g. create a module exporting those functions) and replace the
copies in function.py and struct.py with imports from that shared module so both
files use the same implementations, keeping the function names unchanged to
minimize call-site edits and ensuring any referenced symbols
(OutArrayReturnSpec, to_numba_type_str, to_numba_out_array_type_str,
plan.intents, spec.dtype/length/shim_arg_indirect) remain accessible.
---
Outside diff comments:
In `@numbast/tests/test_callconv.py`:
- Around line 125-183: Add a new test that mirrors the existing out_array_return
tests but sets OutArrayReturnSpec(shim_arg_indirect=False) to exercise the
non-indirect lowering path; create an IntentPlan with
ArgIntent.out_array_return, an appropriate return_type (e.g. UniTuple(dtype,
length)), call _lower_callconv_to_ir with that plan, and assert the IR contains
the aggregate-style alloca and GEP patterns (e.g. an "alloca [N x <elem>]" or
similar aggregate alloca and a "getelementptr" into the aggregate) to lock in
the aggregate array alloca/GEP branch rather than the indirect pointer path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6b5556e3-8572-4524-9b7e-2d839e1baf35
⛔ Files ignored due to path filters (1)
docs/source/generated/static_binding_schema_reference.rstis excluded by!**/generated/**
📒 Files selected for processing (21)
docs/source/argument_intents.rstnumbast/src/numbast/__init__.pynumbast/src/numbast/callconv.pynumbast/src/numbast/class_template.pynumbast/src/numbast/function.pynumbast/src/numbast/function_template.pynumbast/src/numbast/intent.pynumbast/src/numbast/intent_defs.pynumbast/src/numbast/intent_utils.pynumbast/src/numbast/static/function.pynumbast/src/numbast/static/struct.pynumbast/src/numbast/static/tests/data/function_out.cuhnumbast/src/numbast/static/tests/data/src/function_out.cunumbast/src/numbast/static/tests/test_function_static_bindings.pynumbast/src/numbast/static/types.pynumbast/src/numbast/struct.pynumbast/src/numbast/tools/static_binding_generator.schema.yamlnumbast/src/numbast/types.pynumbast/tests/data/sample_function_out.cuhnumbast/tests/test_callconv.pynumbast/tests/test_function.py
Comment on lines
+24
to
+33
| def _get_out_array_return_specs(plan): | ||
| specs = getattr(plan, "out_array_return_specs", ()) | ||
| if not specs: | ||
| return (None,) * len(plan.intents) | ||
| if len(specs) != len(plan.intents): | ||
| raise ValueError( | ||
| "IntentPlan out_array_return_specs length does not match intents: " | ||
| f"{len(specs)} != {len(plan.intents)}" | ||
| ) | ||
| return tuple(specs) |
Contributor
There was a problem hiding this comment.
Fail fast when out_array_return intent lacks a spec.
Line 26 currently treats missing out_array_return_specs as all-None. If an IntentPlan includes out_array_return, this can silently take the scalar out-return path and generate mismatched ABI/lowering artifacts. Please raise immediately when an out_array_return intent has no corresponding spec.
Suggested guard
def _get_out_array_return_specs(plan):
specs = getattr(plan, "out_array_return_specs", ())
- if not specs:
- return (None,) * len(plan.intents)
+ if not specs:
+ if any(getattr(i, "value", i) == "out_array_return" for i in plan.intents):
+ raise ValueError(
+ "IntentPlan contains out_array_return intent but out_array_return_specs is missing"
+ )
+ return (None,) * len(plan.intents)
if len(specs) != len(plan.intents):
raise ValueError(
"IntentPlan out_array_return_specs length does not match intents: "
f"{len(specs)} != {len(plan.intents)}"
)
+ for idx, (intent, spec) in enumerate(zip(plan.intents, specs)):
+ if getattr(intent, "value", intent) == "out_array_return" and spec is None:
+ raise ValueError(
+ f"Missing out_array_return spec for intent index {idx}"
+ )
return tuple(specs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@numbast/src/numbast/callconv.py` around lines 24 - 33, The current
_get_out_array_return_specs returns all-None when out_array_return_specs is
missing, which can mask cases where an IntentPlan contains an out_array_return
intent; change it to fail fast: in _get_out_array_return_specs, if
out_array_return_specs is falsy, scan plan.intents for any intent that
represents the out_array_return (match the enum/constant used in your code,
e.g., Intent.out_array_return or intent.name == "out_array_return"); if any such
intent exists, raise a ValueError indicating a missing out_array_return spec,
otherwise return the tuple of Nones as before; keep the existing length check
(len(specs) != len(plan.intents)) and final return tuple(specs).
Comment on lines
+28
to
+35
| def resolve_out_array_dtype(dtype) -> nbtypes.Type: | ||
| if isinstance(dtype, nbtypes.Type): | ||
| return dtype | ||
| if isinstance(dtype, str): | ||
| resolved = to_numba_type(dtype) | ||
| if not isinstance(resolved, nbtypes.Opaque): | ||
| return resolved | ||
| raise ValueError(f"Unknown out_array_return dtype: {dtype!r}") |
Contributor
There was a problem hiding this comment.
Reject numba.undefined here.
to_numba_type() uses numba.undefined as the unknown-type sentinel in this repo. The current check only filters Opaque, so an invalid dtype string can still flow into UniTuple(...) / CPointer(...) instead of raising the intended ValueError.
💡 Suggested fix
def resolve_out_array_dtype(dtype) -> nbtypes.Type:
if isinstance(dtype, nbtypes.Type):
return dtype
if isinstance(dtype, str):
resolved = to_numba_type(dtype)
- if not isinstance(resolved, nbtypes.Opaque):
+ if resolved is not nbtypes.undefined and not isinstance(
+ resolved, nbtypes.Opaque
+ ):
return resolved
raise ValueError(f"Unknown out_array_return dtype: {dtype!r}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@numbast/src/numbast/intent_utils.py` around lines 28 - 35, The
resolve_out_array_dtype function currently accepts results from to_numba_type
that can be numba.undefined (the sentinel for unknown types) because it only
filters nbtypes.Opaque; update resolve_out_array_dtype to treat numba.undefined
as an invalid resolution: after calling to_numba_type(dtype) check if resolved
is numba.undefined or is an instance of nbtypes.Opaque and, if so, raise the
same ValueError; otherwise return the resolved nbtypes.Type. This refers to
resolve_out_array_dtype, to_numba_type, nbtypes.Opaque and the numba.undefined
sentinel.
Comment on lines
+70
to
+72
| def _is_pointer_type(ast_type: Any) -> bool: | ||
| type_name = getattr(ast_type, "unqualified_non_ref_type_name", "") | ||
| return "*" in str(type_name) |
Contributor
There was a problem hiding this comment.
Recognize native array spellings for out_array_return.
This path only treats * spellings as pointer-like. If AST Canopy preserves a parameter as T [N], Line 198 rejects it even though this feature is meant for fixed-size native output arrays.
💡 Suggested fix
def _is_pointer_type(ast_type: Any) -> bool:
type_name = getattr(ast_type, "unqualified_non_ref_type_name", "")
- return "*" in str(type_name)
+ type_name = str(type_name)
+ return "*" in type_name or ("[" in type_name and "]" in type_name)Also applies to: 191-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@numbast/src/numbast/intent.py` around lines 70 - 72, The helper
_is_pointer_type currently only checks for "*" in getattr(ast_type,
"unqualified_non_ref_type_name"); extend it to also recognize native array
spellings like "T[N]" or "T[]" by matching brackets in the type name (e.g.,
check for "[" and "]" or use a regex like r"\[\s*\d*\s*\]"); update
_is_pointer_type accordingly so the out_array_return flow (and the logic
referenced around out_array_return) treats fixed-size/native arrays as
pointer-like and is accepted where "*" is currently detected.
Comment on lines
+67
to
+122
| def _tuple_literal(items: list[str]) -> str: | ||
| if not items: | ||
| return "()" | ||
| if len(items) == 1: | ||
| return f"({items[0]},)" | ||
| return f"({', '.join(items)})" | ||
|
|
||
|
|
||
| def _out_array_specs_for_plan(plan): | ||
| specs = getattr(plan, "out_array_return_specs", ()) | ||
| if not specs: | ||
| return (None,) * len(plan.intents) | ||
| if len(specs) != len(plan.intents): | ||
| raise ValueError( | ||
| "IntentPlan out_array_return_specs length does not match intents: " | ||
| f"{len(specs)} != {len(plan.intents)}" | ||
| ) | ||
| return tuple(specs) | ||
|
|
||
|
|
||
| def _out_return_type_str(param_type, spec) -> str: | ||
| if spec is None: | ||
| return to_numba_type_str(param_type.unqualified_non_ref_type_name) | ||
| return to_numba_out_array_type_str(spec.dtype, spec.length) | ||
|
|
||
|
|
||
| def _compose_return_type_str( | ||
| cxx_return_type_str: str, out_return_types: list[str] | ||
| ): | ||
| if not out_return_types: | ||
| return cxx_return_type_str | ||
| if cxx_return_type_str == "void": | ||
| if len(out_return_types) == 1: | ||
| return out_return_types[0] | ||
| outs = ", ".join(out_return_types) | ||
| return f"types.Tuple(({outs},))" | ||
| outs = ", ".join([cxx_return_type_str, *out_return_types]) | ||
| return f"types.Tuple(({outs},))" | ||
|
|
||
|
|
||
| def _render_out_array_specs(plan) -> str: | ||
| specs = _out_array_specs_for_plan(plan) | ||
| rendered = [] | ||
| for spec in specs: | ||
| if spec is None: | ||
| rendered.append("None") | ||
| else: | ||
| dtype_str = to_numba_type_str(spec.dtype) | ||
| rendered.append( | ||
| "OutArrayReturnSpec(" | ||
| f"dtype={dtype_str}, " | ||
| f"length={int(spec.length)}, " | ||
| f"shim_arg_indirect={bool(spec.shim_arg_indirect)}" | ||
| ")" | ||
| ) | ||
| return _tuple_literal(rendered) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Deduplicate these out-array rendering helpers.
This block is effectively copied into numbast/src/numbast/static/struct.py. Keeping two string-rendering implementations for the same out_array_return behavior makes function and method bindings drift the next time this format changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@numbast/src/numbast/static/function.py` around lines 67 - 122, The out-array
string-rendering helpers (_tuple_literal, _out_array_specs_for_plan,
_out_return_type_str, _compose_return_type_str, _render_out_array_specs) are
duplicated in struct.py; extract them into a single shared module (e.g. create a
module exporting those functions) and replace the copies in function.py and
struct.py with imports from that shared module so both files use the same
implementations, keeping the function names unchanged to minimize call-site
edits and ensuring any referenced symbols (OutArrayReturnSpec,
to_numba_type_str, to_numba_out_array_type_str, plan.intents,
spec.dtype/length/shim_arg_indirect) remain accessible.
isVoid
commented
May 29, 2026
Comment on lines
+279
to
+284
| Function Argument Intents: | ||
| get_matrix: | ||
| out: | ||
| intent: out_array_return | ||
| dtype: float | ||
| length: 12 |
Collaborator
Author
There was a problem hiding this comment.
The reason behind this design is because of the lack of fixed-size array return information from ast_canopy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a Numbast-level out_array_return(dtype=..., length=...) argument intent for fixed-size native output arrays such as float out[12] and float4 out[3].
The new intent hides the native output pointer from the Python-facing signature, allocates native stack storage in lowering, passes the raw storage pointer through the shim, loads each element after the call, and returns a fixed UniTuple value. It also updates dynamic/static binding paths, function templates, struct method helpers, config schema/docs, and focused tests.
Validation
CUDA runtime validation attempted with numbast/tests/test_function.py::test_out_return_device_function_results, but this machine has no CUDA device: driver init failed with CUDA_ERROR_NO_DEVICE.
Summary by CodeRabbit
New Features
Documentation
Tests
Schema