Fix AARCH64 emitting invalid asm in CUDA kernels.

[duburcqa]

Problem

On AArch64 + CUDA (e.g. NVIDIA GB10 / Jetson Thor), any Genesis kernel that pulls in cuda_active_mask, cuda_match_all_sync_i32, or cuda_match_any_sync_i64 crashes during LLVM PTX code generation with:

error: couldn't allocate output register for constraint 'w' at line 37014

Root cause: runtime.cpp contains PTX inline assembly for these three functions, but uses #if aarch64 guards to select AArch64 register constraints ("=w" for 32-bit FP/SIMD registers) instead of x86 ones ("=r"). When clang compiles runtime.cpp
to bitcode on AArch64, it embeds the "=w" constraint. Later, when this bitcode is linked into an NVPTX module, the LLVM NVPTX backend rejects "=w" — it is a valid AArch64 constraint but meaningless in PTX.

Separately, the CPU backend also fails on AArch64 with Function "runtime_initialize" not found, because clang's default -moutline-atomics emits calls to __aarch64_ldadd4_acq_rel etc. that the ORC JIT linker cannot resolve.

Solution

Commit 1: Replace the three inline-asm functions with trivial stubs (return 0) and add corresponding patch_intrinsic calls in llvm_context.cpp to replace them with proper LLVM NVPTX intrinsics (nvvm_activemask, nvvm_match_all_sync_i32p,
nvvm_match_any_sync_i32) at module-init time. This is the same mechanism already used for dozens of other CUDA intrinsics in the same file. The old patch_intrinsic calls for these were commented out due to LLVM 10 issues that are long fixed in
LLVM 22.

Commit 2: Add -mno-outline-atomics to the clang flags that compile runtime.cpp to bitcode, so atomic operations use inline instructions instead of external helper calls.

Platform impact

• AArch64 + CUDA: fixes the PTX codegen crash (the original bug).
• AArch64 + CPU: fixes the JIT linker crash (outline atomics).
• x86_64 + CUDA: no behavioral change. patch_intrinsic produces the same NVPTX IR that the old inline asm did. CI tests (test_active_mask, test_match_any, test_match_all in test_simt.py) cover this.
• x86_64 + CPU: no change. -mno-outline-atomics is a no-op on x86.

Note on cuda_match_any_sync_i64

This function has a stub in runtime.cpp but is not registered in internal_ops.inc.h or type_system.cpp, so no codegen path emits calls to it. This was true before this PR as well (the old inline asm was also dead code). No patch_intrinsic is
added for it. A comment documents why.

[hughperkins]

Test process? I'm guessing this makes Genesis kernels pass, which used to fail? Could we add some relevant quadrants tests please.

[hughperkins]

Oh I guess we would need runners that are both:

• arm
• have a cuda gpu
  🤔

[hughperkins]

Could you provide a description please of:

• what challenge you encountered?
• on which specific platforms (is it just aarch + cuda?)
• how this PR intends to solve this?
• how we can be fairly sure that this PR won't break things on other platform combinations?

[claude]

LGTM — cleanly replaces architecture-guarded inline asm with LLVM intrinsics via the established patch_intrinsic pattern.

Extended reasoning...

Overview

Two files changed: llvm_context.cpp (adds three new patch_intrinsic calls for cuda_active_mask, cuda_match_all_sync_i32, and cuda_match_any_sync_i64) and runtime_module/runtime.cpp (removes the corresponding architecture-conditional asm volatile blocks, leaving plain stubs). The net effect is that AARCH64-host builds no longer emit PTX-incompatible inline assembly for these three CUDA warp-match / active-mask intrinsics.

Security Risks

None. This is internal GPU runtime JIT compilation code with no auth, crypto, or user-facing attack surface.

Level of Scrutiny

Medium. The code touches CUDA kernel compilation, where a wrong intrinsic or wrong argument order could silently produce bad PTX. That said, the implementation follows the exact pattern used for every other CUDA intrinsic in the same file. The cuda_match_all_sync_i32 case correctly handles the {i32, i1} struct return of nvvm_match_all_sync_i32p by extracting element 0, which is the only non-trivial piece of logic in the diff.

Other Factors

The one flagged bug (duplicate patch_intrinsic("block_memfence",...) call) is pre-existing and carries zero semantic impact — both calls produce identical IR. It was not introduced by this PR. No existing reviewer comments are outstanding. The change is small, self-contained, and directly addresses the stated AARCH64 asm-constraint mismatch.

[hughperkins]

my converastion with opus:

why didnt we use patch intrinscis before?

Opus:

  The old code on main tells the story clearly. There were two distinct reasons:


  1. `cuda_match_all_sync_i32` — return type mismatch

  The comments say "LLVM 10.0.0 seems to have a bug on this intrinsic function." Looking more closely, it wasn't really a bug — it's a type mismatch. The LLVM intrinsic
  nvvm_match_all_sync_i32p returns {i32, i1} (a struct with the matched value and a predicate), but the C stub cuda_match_all_sync_i32 is declared as returning plain u32. The
  simple patch_intrinsic lambda just does CreateRet(CreateIntrinsic(...)), which doesn't work when the intrinsic's return type doesn't match the function's return type. You'd
  need to ExtractValue first — which is exactly what this PR adds.
  So back in the LLVM 10 era, someone tried it, it didn't work, assumed it was an LLVM bug, and fell back to inline PTX assembly instead. The commented-out code also has a typo
  (nvvm_math_all_sync_i32 instead of nvvm_match_all_sync_i32p), suggesting it was never fully working.


  2. `cuda_match_any_sync_i64` and `cuda_active_mask` — likely same era workaround

  These were also commented out with the same "LLVM 10.0.0 bug" note. They may have genuinely been buggy in LLVM 10, or they may have been collateral — disabled at the same time
   as match_all and replaced with inline asm as a batch. The inline asm worked fine on x86_64, so nobody revisited it until the project needed to build on AARCH64, where the
  register constraints ("=w" vs "=r") are different and the old asm was wrong.


  TL;DR

  The original patch_intrinsic approach was tried but abandoned because (a) match_all returns a struct that the generic helper couldn't handle, (b) the other two were assumed
  buggy in the old LLVM version, and (c) inline asm worked fine on x86_64 so there was no pressure to fix it. The AARCH64 port exposed the inline asm as broken, and this PR
  finally does what should have been done all along — use the proper LLVM intrinsics, with a custom IR wrapper for the match_all struct return.

what does match all do?

Opus:

  cuda_match_all_sync_i32 is a CUDA warp-level primitive. Given a warp mask and a value, it checks whether all active threads (identified by the mask) hold the same value. It
  returns:
  • The matching value (as i32) — which is just the common value if all threads agree.
  • A predicate (i1) — true if all active threads had the same value, false otherwise.

  The LLVM intrinsic (nvvm_match_all_sync_i32p) returns both as a struct {i32, i1}, but this codebase only cares about the i32 part, which is why the PR adds the
  ExtractValue(result, {0}) wrapper to discard the predicate.
  Its sibling match_any_sync is the more general variant — it returns a mask of which threads share the same value as the calling thread, without requiring unanimity.
  These are used for things like warp-level reductions, deduplication, and divergence detection in GPU kernels.

is there anything that could go wrong in this pr, or as a rsul of merging this pr?

Opus:

1. `cuda_match_all_sync_i32` — the predicate is silently dropped

  The NVVM intrinsic nvvm_match_all_sync_i32p returns {i32, i1} where the i1 is a predicate indicating whether all threads actually matched. This PR extracts only element {0}
  (the i32 value) and discards the predicate. The Python API warp.match_all() also only returns one value.
  This means there's no way for user code to know whether all threads actually agreed. The returned i32 is the value itself, but without the predicate it's ambiguous — if
  threads didn't all match, the return value is unspecified per CUDA semantics. If anyone is relying on this function to detect unanimity (which is the whole point of match_all
  vs match_any), they'd have no way to do so. This isn't a regression though — the old inline asm also dropped the predicate.


  2. `cuda_match_any_sync_i64` — not registered as an internal op

  This is the most concrete concern. The function cuda_match_any_sync_i64 has a C stub in runtime.cpp and is now patched with patch_intrinsic in llvm_context.cpp, but it is not
  listed in internal_ops.inc.h or type_system.cpp. There's no CUDA_MATCH_SYNC(any, i64) and no PER_INTERNAL_OP(cuda_match_any_sync_i64). There's also no Python-level match_any
  variant that takes i64.
  This means the patch_intrinsic call for cuda_match_any_sync_i64 will likely never find a function in the module (since no codegen path emits calls to it), and the code is
  dead. Not harmful, but also not useful — and if someone later adds the i64 variant to the IR, they might not realize the C stub is a no-op return 0 and would silently get
  wrong results on non-CUDA backends.


  3. CPU backend — stubs return 0

  The stubs now unconditionally return 0 on all architectures. Before, on CUDA they used inline asm, and on non-CUDA they returned 0. The new code relies entirely on
  patch_intrinsic to replace the body at LLVM IR level for CUDA. If the patching somehow fails (e.g., function name mismatch, module doesn't contain the function), the stub
  returning 0 would silently execute on GPU with wrong results. The old inline asm would at least produce the correct PTX. That said, this failure mode is unlikely since
  match_any_sync_i32 already uses patch_intrinsic on main and works fine.


  4. Behavioral change for `cuda_active_mask` and `cuda_match_any_sync_i64`

  These were previously implemented with inline asm on CUDA and were actually functional at runtime. Now they rely on patch_intrinsic. If the patching works (and it should — the
   same pattern is used for dozens of other intrinsics), this is a clean improvement. But it is a change in code generation path, so any subtle differences in how LLVM lowers
  the intrinsic vs the inline asm could surface. For example, the inline asm was marked volatile, preventing reordering — the LLVM intrinsics may have different scheduling
  behavior.


  Summary

  • Real risk: Essentially none for correctness, assuming your LLVM version supports these intrinsics (you're on LLVM 22, so yes).
  • Dead code: The cuda_match_any_sync_i64 patch is unreachable since there's no IR/type registration for it.
  • Design gap (pre-existing): match_all discards the predicate, making it functionally questionable, but that's not a regression from this PR.

  The PR looks safe to merge. The main actionable feedback would be to either remove the dead cuda_match_any_sync_i64 code or properly register it in internal_ops.inc.h and
  type_system.cpp.

[hughperkins]

what tests are there for this function? how do we know it continues to work ok on cuda? Sorry on cuda x86

Opus:

  Test coverage

  There are three relevant tests in tests/python/test_simt.py, all gated to arch=qd.cuda:
  • `test_match_any` — verifies match_any returns correct thread bitmasks for two groups of values
  • `test_match_all` — verifies match_all returns 0xFFFFFFFF when all threads agree, and 0 when they don't
  • `test_active_mask` — verifies active_mask returns the correct mask for 16 active threads

  Plus a lower-level test_active_mask in test_cuda_internals.py.


  CI coverage on CUDA x86

  The test_gpu.yml workflow has a test_linux_cuda job that runs on gpu-t4-4-core runners (NVIDIA T4, x86_64). It runs:

  python tests/run_tests.py -r 1 -v --arch cuda

  This runs all Python tests under tests/python/ filtered to CUDA arch, which includes test_simt.py. The T4 is compute capability 75, so the >= 70 guards in test_match_any and
  test_match_all will pass and the tests will execute.


  So: yes, CI should catch regressions on CUDA x86

  As long as the GPU test workflow runs on this PR (it's triggered via workflow_call, so it depends on the parent workflow), those three tests will exercise all three changed
  functions on a real T4 GPU on x86_64. If the intrinsic patching produces different results than the old inline asm, the assertions would fail.
  The main gap is that there are no tests for `cuda_match_any_sync_i64` — but as I noted earlier, that function isn't wired up in the IR/type system anyway, so it's dead code on
   both main and this branch.

[hughperkins]

@duburcqa please could you address Opus's comments on cuda_match_any_sync_i64

[duburcqa]

@hughperkins Addressed — cuda_match_any_sync_i64 is not registered in internal_ops.inc.h or type_system.cpp, so no codegen path emits calls to it. This was already the case before this PR (the old inline asm was dead code too). I've removed the patch_intrinsic call for it and added a comment explaining why it's skipped. The stub in runtime.cpp remains (harmless, and removing it would be a separate cleanup).

Updated the PR description with the full context: what broke, on which platforms, how it's fixed, and why it's safe on x86_64.

Force-pushed to update the commits.

[duburcqa]

Test process? I'm guessing this makes Genesis kernels pass, which used to fail? Could we add some relevant quadrants tests please.

pytest -xsv -n 0 "tests/python/test_simt.py::test_active_mask" "tests/python/test_simt.py::test_match_any" "tests/python/test_simt.py::test_match_all"
[Quadrants] version 0.0.0, llvm 22.1.0, commit 13f18a5b, linux, python 3.12.3
================================================================================================================= test session starts ==================================================================================================================
platform linux -- Python 3.12.3, pytest-9.0.2, pluggy-1.6.0 -- /home/genesis-ai/workspace/src/genesis/.venv/bin/python3
cachedir: .pytest_cache
rootdir: /home/genesis-ai/workspace/src/quadrants/tests
configfile: pytest.ini
plugins: xdist-3.8.0, repeat-0.9.4, forked-1.6.0, rerunfailures-16.1, anyio-4.12.1, print-1.2.2, timeout-2.4.0, syrupy-5.1.0
collected 3 items                                                                                                                                                                                                                                      

tests/python/test_simt.py::test_active_mask[arch=cuda] [Quadrants] Starting on arch=cuda
error: couldn't allocate output register for constraint 'w' at line 37014

[duburcqa]

For the other fix, any atomic unit test in tests/python/test_atomic.py could trigger the crash:

tests/python/test_atomic.py::test_atomic_add_global_i32[arch=arm64-1] [Quadrants] Starting on arch=arm64
JIT session error: Symbols not found: [ __aarch64_ldadd4_acq_rel, __aarch64_ldadd8_acq_rel, __aarch64_swp4_acq_rel, __aarch64_swp8_acq_rel ]
[E 04/13/26 13:11:42.189 211990] [jit_cpu.cpp:lookup_in_module@198] Function "runtime_initialize" not found


ERROR

======================================================================================================================== ERRORS ========================================================================================================================
______________________________________________________________________________________________ ERROR at setup of test_atomic_add_global_i32[arch=arm64-1] ______________________________________________________________________________________________

request = <SubRequest 'wanted_arch' for <Function test_atomic_add_global_i32[arch=arm64-1]>>, req_arch = <Arch.arm64: 1>, req_options = {'print_full_traceback': True}

    @pytest.fixture(autouse=True)
    def wanted_arch(request, req_arch, req_options):
        if req_arch is not None:
            if req_arch == qd.cuda:
                if not request.node.get_closest_marker("run_in_serial"):
                    # Optimization only apply to non-serial tests, since serial tests
                    # are picked out exactly because of extensive resource consumption.
                    # Separation of serial/non-serial tests is done by the test runner
                    # through `-m run_in_serial` / `-m not run_in_serial`.
                    req_options = {
                        "device_memory_GB": 0.3,
                        "cuda_stack_limit": 1024,
                        **req_options,
                    }
                else:
                    # Serial tests run without aggressive resource optimization
                    req_options = {"device_memory_GB": 1, **req_options}
            if "print_full_traceback" not in req_options:
                req_options["print_full_traceback"] = True
>           qd.init(arch=req_arch, enable_fallback=False, **req_options)

tests/python/conftest.py:73: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

arch = <Arch.arm64: 1>, default_fp = None, default_ip = None, _test_mode = False, enable_fallback = False, require_version = None, print_non_pure = False, src_ll_cache = True, kwargs = {}, current_dir = '/home/genesis-ai/workspace/src/quadrants'
cfg = <quadrants._lib.core.quadrants_python.CompileConfig object at 0xfb8d5fcbb830>, spec_cfg = <quadrants.lang.misc._SpecialConfig object at 0xfb8e19d37a70>, env_comp = <quadrants.lang.misc._EnvironmentConfigurator object at 0xfb8d5f2f9be0>
env_spec = <quadrants.lang.misc._EnvironmentConfigurator object at 0xfb8d5f2fa9f0>, env_default_fp = None, env_default_ip = None

    def init(
        arch=None,
        default_fp=None,
        default_ip=None,
        _test_mode: bool = False,
        enable_fallback: bool = True,
        require_version: str | None = None,
        print_non_pure: bool = False,
        src_ll_cache: bool = True,
        **kwargs,
    ):
        """Initializes the Quadrants runtime.
    
        This should always be the entry point of your Quadrants program. Most
        importantly, it sets the backend used throughout the program.
    
        Args:
            arch: Backend to use. This is usually :const:`~quadrants.lang.cpu` or :const:`~quadrants.lang.gpu`.
            default_fp (Optional[type]): Default floating-point type.
            default_ip (Optional[type]): Default integral type.
            require_version: A version string.
            print_non_pure: Print the names of kernels, at the time they are executed, which are not annotated with
                            @qd.pure
            src_ll_cache: enable SRC-LL-CACHE, which will accelerate loading from cache, across all architectures,
                          for pure kernels (i.e. kernels declared as @qd.pure)
            **kwargs: Quadrants provides highly customizable compilation through
                ``kwargs``, which allows for fine grained control of Quadrants compiler
                behavior. Below we list some of the most frequently used ones. For a
                complete list, please check out
                https://github.com/Genesis-Embodied-AI/quadrants/blob/master/quadrants/program/compile_config.h.
    
                * ``cpu_max_num_threads`` (int): Sets the number of threads used by the CPU thread pool.
                * ``debug`` (bool): Enables the debug mode, under which Quadrants does a few more things like boundary checks.
                * ``print_ir`` (bool): Prints the CHI IR of the Quadrants kernels.
                *``offline_cache`` (bool): Enables offline cache of the compiled kernels. Default to True. When this is enabled Quadrants will cache compiled kernel on your local disk to accelerate future calls.
                *``random_seed`` (int): Sets the seed of the random generator. The default is 0.
                *``debug_dump_path`` (str): used as the base path for QD_DUMP_IR and similar
        """
        # FIXME(https://github.com/taichi-dev/taichi/issues/4811): save the current working directory since it may be
        # changed by the Vulkan backend initialization on OS X.
        current_dir = os.getcwd()
    
        # Check if installed version meets the requirements.
        if require_version is not None:
            check_require_version(require_version)
    
        if "default_up" in kwargs:
            raise KeyError("'default_up' is always the unsigned type of 'default_ip'. Please set 'default_ip' instead.")
        # Make a deepcopy in case these args reference to items from qd.cfg, which are
        # actually references. If no copy is made and the args are indeed references,
        # qd.reset() could override the args to their default values.
        default_fp = _deepcopy(default_fp)
        default_ip = _deepcopy(default_ip)
        kwargs = _deepcopy(kwargs)
        reset()
    
        cfg = impl.default_cfg()
        cfg.offline_cache = True  # Enable offline cache in frontend instead of C++ side
    
        spec_cfg = _SpecialConfig()
        env_comp = _EnvironmentConfigurator(kwargs, cfg)
        env_spec = _EnvironmentConfigurator(kwargs, spec_cfg)
    
        # configure default_fp/ip:
        # TODO: move these stuff to _SpecialConfig too:
        env_default_fp = os.environ.get("QD_DEFAULT_FP")
        if env_default_fp:
            if default_fp is not None:
                _qd_core.warn(
                    f'Environment variable QD_DEFAULT_FP={env_default_fp} overridden by qd.init argument "default_fp"'
                )
            elif env_default_fp == "32":
                default_fp = f32
            elif env_default_fp == "64":
                default_fp = f64
            elif env_default_fp is not None:
                raise ValueError(f"Invalid QD_DEFAULT_FP={env_default_fp}, should be 32 or 64")
    
        env_default_ip = os.environ.get("QD_DEFAULT_IP")
        if env_default_ip:
            if default_ip is not None:
                _qd_core.warn(
                    f'Environment variable QD_DEFAULT_IP={env_default_ip} overridden by qd.init argument "default_ip"'
                )
            elif env_default_ip == "32":
                default_ip = i32
            elif env_default_ip == "64":
                default_ip = i64
            elif env_default_ip is not None:
                raise ValueError(f"Invalid QD_DEFAULT_IP={env_default_ip}, should be 32 or 64")
    
        if default_fp is not None:
            impl.get_runtime().set_default_fp(default_fp)
        if default_ip is not None:
            impl.get_runtime().set_default_ip(default_ip)
    
        # submodule configurations (spec_cfg):
        env_spec.add("log_level", str)
        env_spec.add("gdb_trigger")
        env_spec.add("short_circuit_operators")
        env_spec.add("print_full_traceback")
        env_spec.add("unrolling_limit")
    
        # compiler configurations (qd.cfg):
        for key in dir(cfg):
            if key in ["arch", "default_fp", "default_ip"]:
                continue
            _cast = type(getattr(cfg, key))
            if _cast is bool:
                _cast = None
            env_comp.add(key, _cast)
    
        unexpected_keys = kwargs.keys()
    
        if len(unexpected_keys):
            raise KeyError(f'Unrecognized keyword argument(s) for qd.init: {", ".join(unexpected_keys)}')
    
        if (cfg.print_ir or os.getenv("QD_DUMP_IR") == "1") and cfg.offline_cache:
            util.warning(
                "Even with print_ir/QD_DUMP_IR enabled, already cached kernels won't get their IRs shown. "
                "You might want to disable caching with offline_cache=False. "
                "[warning_code=DUMP_IR_CACHE_MISMATCH]"
            )
    
        # dispatch configurations that are not in qd.cfg:
        runtime = impl.get_runtime()
        if not _test_mode:
            _qd_core.set_core_trigger_gdb_when_crash(spec_cfg.gdb_trigger)
            runtime.short_circuit_operators = spec_cfg.short_circuit_operators
            runtime.print_full_traceback = spec_cfg.print_full_traceback
            runtime.unrolling_limit = spec_cfg.unrolling_limit
            runtime.src_ll_cache = src_ll_cache
            runtime.print_non_pure = print_non_pure
            _logging.set_logging_level(spec_cfg.log_level.lower())
    
        # select arch (backend):
        env_arch = os.environ.get("QD_ARCH")
        if env_arch is not None:
            _logging.info(f"Following QD_ARCH setting up for arch={env_arch}")
            arch = _qd_core.arch_from_name(env_arch)
        cfg.arch = adaptive_arch_select(arch, enable_fallback)
        print(f"[Quadrants] Starting on arch={_qd_core.arch_name(cfg.arch)}")
    
        if cfg.arch == _qd_core.amdgpu and get_os_name() == "win":
            _logging.warn("AMDGPU support on Windows is experimental and may not work as expected.")
    
        if _test_mode:
            return spec_cfg
    
        get_default_kernel_profiler().set_kernel_profiler_mode(cfg.kernel_profiler)
    
        impl.get_runtime()._arch = cfg.arch
    
        # create a new program (skip for python backend — no C++ runtime needed):
        if cfg.arch != _qd_core.python:
            impl.get_runtime().create_program()
            _logging.trace("Materializing runtime...")
>           impl.get_runtime().prog.materialize_runtime()
E           RuntimeError: [jit_cpu.cpp:lookup_in_module@198] Function "runtime_initialize" not found

python/quadrants/lang/misc.py:485: RuntimeError
=============================================================================================================== short test summary info ================================================================================================================
ERROR tests/python/test_atomic.py::test_atomic_add_global_i32[arch=arm64-1] - RuntimeError: [jit_cpu.cpp:lookup_in_module@198] Function "runtime_initialize" not found
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
============================================================================================================== 1 passed, 1 error in 0.59s ==============================================================================================================
Bus error (core dumped)

[hughperkins]

Thanks! Seems there are some faliures in test_pointer https://github.com/Genesis-Embodied-AI/quadrants/actions/runs/24339347087/job/71067191548?pr=473 Re-running, in case it's an infra issue.

[duburcqa]

Fixed. Removing 'cuda_match_any_sync_i64' was a mistake. I documented this in the code.

[duburcqa]

@claude review

[claude]

LGTM — the inline-asm → LLVM-intrinsic replacement is the right approach, the custom IR wrapper for cuda_match_all_sync_i32's struct return correctly extracts element 0, and -mno-outline-atomics cleanly fixes the CPU JIT crash.

Extended reasoning...

Overview

Three files changed: runtime.cpp removes arch-specific PTX inline asm and replaces three functions with trivial stubs; llvm_context.cpp adds corresponding patch_intrinsic calls (plus a custom IR builder for the struct-returning nvvm_match_all_sync_i32p); CMakeLists.txt adds -mno-outline-atomics to the bitcode compilation flags.

Security risks

None. This is a LLVM bitcode compilation and JIT-linking fix with no auth, crypto, or permission surfaces.

Level of scrutiny

Moderate. The patch_intrinsic pattern is used extensively in llvm_context.cpp for dozens of other intrinsics, so the approach is well-established. The one novel piece is the custom IR builder for cuda_match_all_sync_i32, which needs to handle the {i32, i1} struct return of nvvm_match_all_sync_i32p — the CreateExtractValue(result, {0}) call correctly pulls the i32 match value, matching the old inline-asm behavior of discarding the predicate.

Other factors

• The author provided concrete crash reproduction and test output for both failure modes.
• Existing test_active_mask, test_match_any, and test_match_all in test_simt.py cover the CUDA x86 path on CI T4 runners.
• The pre-existing __linux___ typo (three underscores) flagged by the bug hunter is rendered moot by -mno-outline-atomics; the fix works regardless.
• The pre-existing duplicate block_memfence call (flagged in a prior review) is harmless and out of scope for this PR.
• The author addressed the cuda_match_any_sync_i64 concern by restoring the patch_intrinsic call with a comment explaining it IS called from C++ runtime code in node_pointer.h::is_representative().

[claude]

LGTM — the inline-asm stubs are correctly replaced with LLVM NVPTX intrinsics, the dead AArch64 atomic helper block is removed, and -mno-outline-atomics cleanly solves the JIT linker issue.

Extended reasoning...

Overview

This PR touches three files: llvm_context.cpp (adds patch_intrinsic calls for cuda_active_mask, cuda_match_all_sync_i32, and cuda_match_any_sync_i64), CMakeLists.txt (adds -mno-outline-atomics to the clang command that compiles runtime bitcode), and runtime.cpp (replaces PTX inline-asm bodies with trivial stubs and removes the dead aarch64 atomic helper block).

Security risks

None. These are internal LLVM IR lowering helpers with no security surface.

Level of scrutiny

Moderate — this touches GPU codegen, but the patch_intrinsic pattern is already used for ~30 other intrinsics in the same file, making this a well-understood, low-risk extension of an existing mechanism. The custom IR wrapper for cuda_match_all_sync_i32 (ExtractValue from a struct return) is straightforward and correct. The -mno-outline-atomics flag is a no-op on x86 and documented with a clear rationale.

Other factors

Prior inline comments from this reviewer flagged (1) the duplicate block_memfence call (pre-existing, out of scope) and (2) the linux_ typo — the dead block has been fully removed in this PR, resolving that concern. The cuda_match_any_sync_i64 patch_intrinsic was re-added after the author confirmed it is live in node_pointer.h::is_representative(). CI tests (test_active_mask, test_match_any, test_match_all) cover the x86_64+CUDA path; the author provided direct test output demonstrating both crash modes on AArch64.

[claude]

LGTM — the patch_intrinsic approach is correct and follows established patterns; the -mno-outline-atomics fix is well-justified.

Extended reasoning...

Overview

Three files changed: CMakeLists.txt adds -mno-outline-atomics to the bitcode compile flags; runtime.cpp removes dead PTX inline-asm blocks and replaces them with trivial stubs; llvm_context.cpp adds patch_intrinsic calls for cuda_active_mask, cuda_match_all_sync_i32 (with a custom IR builder to handle the {i32,i1} struct return), and cuda_match_any_sync_i64.

Security risks

None. This is GPU backend codegen, not auth or data-access code.

Level of scrutiny

Moderate — GPU codegen is non-trivial, but every change follows patterns already used dozens of times in the same file. The custom IR for match_all (CreateIntrinsic + CreateExtractValue) is straightforward. The author documented the rationale thoroughly and reproduced both crash modes. CI covers x86+CUDA via the T4 runner.

Other factors

The only flagged bug (CMakeLists ${arch} vs ${rtm_arch} in the install() line) is pre-existing and not touched by this PR. My two prior inline comments have been acknowledged. The author addressed all reviewer feedback (removed spurious cuda_match_any_sync_i64 patch_intrinsic, then correctly re-added it after finding it is called from node_pointer.h). No outstanding unresolved concerns.

[duburcqa]

I was assisted by Claude Opus to write this PR. I have read and review every changes in this PR. I take full responsibility for the lines added and removed in this PR. I won't blame any issue on Claude Opus. I have reviewed this PR, and approve it.

[hughperkins]

Thanks! 🙌

[claude]

🟡 The comment on CMakeLists.txt line 10 states that -mno-outline-atomics is 'irrelevant for CUDA/GPU paths', but this is factually incorrect — the flag is required for CUDA bitcode on AArch64 for the same reason it is required for CPU. The code itself is correct (the flag is applied to all architectures), but the inaccurate comment could mislead a future developer into removing the flag from CUDA-only builds, reintroducing an NVPTX backend crash; the phrase 'irrelevant for CUDA/GPU paths' should be dropped.

Extended reasoning...

The comment added by this PR reads: 'It is a no-op on x86 and irrelevant for CUDA/GPU paths.' While the first half is accurate, the second half is wrong in a way that creates a latent regression risk.

Why the flag is needed for CUDA bitcode on AArch64: runtime_cuda.bc is compiled from runtime.cpp on the HOST platform (AArch64) with -D ARCH_cuda. The file unconditionally includes atomic.h, which defines atomic_exchange_i32/atomic_exchange_u64 using atomic_exchange builtins (and similarly for atomic_and*, atomic_or, atomic_compare_exchange_ variants). On AArch64 without -mno-outline-atomics, clang lowers these GCC/Clang builtins to calls to __aarch64_swp4_acq_rel and __aarch64_swp8_acq_rel at the LLVM IR level, embedding them as undefined external references in the bitcode.

What patch_atomic_add does NOT cover: The patch_atomic_add lambda in module_from_file only patches atomic_add_i32, atomic_add_i64, atomic_add_f32, and atomic_add_f64. It leaves atomic_exchange_i32, atomic_exchange_u64, and all the atomic_and_/atomic_or_/atomic_xor_/atomic_min_/atomic_max_/atomic_compare_exchange_ variants completely unpatched. Without -mno-outline-atomics, these unpatched functions would retain __aarch64_swp* and __aarch64_ldadd* external calls in runtime_cuda.bc.

The crash path: module_from_file sets the CUDA module target triple to nvptx64-nvidia-cuda and links it with libdevice. Functions like mutex_lock_i32 and mutex_unlock_i32 (which call atomic_exchange_i32) are reachable from CUDA kernels via locked_task (used in allocate_from_reserved_memory, ListManager::touch_chunk, etc.), and ListManager::touch_chunk also calls atomic_exchange_u64. After eliminate_unused_functions retains anything reachable from runtime_* entry points, the __aarch64_swp* calls would survive into the NVPTX module. The NVPTX backend would then encounter unresolvable AArch64-specific external symbols and crash.

Step-by-step proof: (1) On AArch64, clang compiles atomic_exchange_i32 (from atomic.h) to an IR call to _aarch64_swp4_acq_rel when -moutline-atomics is active. (2) mutex_lock_i32 calls atomic_exchange_i32; locked_task calls mutex_lock_i32; allocate_from_reserved_memory calls locked_task; runtime_initialize calls allocate_from_reserved_memory. (3) runtime_initialize is a runtime* symbol, so it survives eliminate_unused_functions. (4) The __aarch64_swp4_acq_rel undefined reference thus survives into the NVPTX-targeted module. (5) The NVPTX backend rejects it and crashes.

Why the current code is actually correct: The -mno-outline-atomics flag IS applied uniformly to all architectures via the single add_custom_target command, so runtime_cuda.bc is compiled correctly with the flag present. The bug is purely in the comment, not the build logic.

The risk and fix: The inaccurate comment was introduced by this PR. A future developer reading 'irrelevant for CUDA/GPU paths' while refactoring the build might guard the flag with 'if(NOT CUDA_ARCH)', reintroducing the crash on any AArch64 + CUDA system. The fix is a one-line comment correction: remove the phrase 'and irrelevant for CUDA/GPU paths', leaving 'It is a no-op on x86.'

Addressing the refutation: The sole refutation agrees the comment is technically inaccurate but argues it is only a comment issue, not a code bug. That is correct — severity is nit, not normal. However, since this inaccurate comment was introduced by this PR and specifically describes the flag as safe to omit for GPU builds when it is not, it warrants a review note.