Repository navigation
[BugFix] Resolve partial scalar reduce barrier participation - #2777
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
|
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:
📝 WalkthroughWalkthroughScalar AllReduce lowering now resolves partial warp-aligned CTA participation on SM90+, tracks per-kernel partial reductions, and emits matching CUDA named-barrier participant counts. ROCm signatures remain compatible, while CUDA tests cover supported and rejected layouts. ChangesPartial scalar AllReduce barriers
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
src/backend/common/op/reduce.h (1)
1121-1127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttribute key is a cross-file contract duplicated as a raw literal. The producer writes the raw string while the consumer declares its own
constexprcopy; a typo on either side silently disables the barrier fix instead of failing loudly.
src/backend/common/op/reduce.h#L1121-L1127: use a shared constant instead of the inline"tl.deferred_scalar_allreduce_barrier"literal.src/transform/lower_tile_op.cc#L45-L46: movekDeferredScalarAllReduceBarrierinto a shared header (e.g. alongside the othertl::attrkeys) and reference it from both sites.🤖 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 `@src/backend/common/op/reduce.h` around lines 1121 - 1127, The attribute key is duplicated as a raw literal and a local constexpr, allowing producer and consumer names to diverge. In src/transform/lower_tile_op.cc lines 45-46, move kDeferredScalarAllReduceBarrier into a shared header alongside the existing tl::attr keys; in src/backend/common/op/reduce.h lines 1121-1127, replace the inline string with that shared constant, and update the consumer to reference the same definition.
🤖 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 `@src/transform/lower_tile_op.cc`:
- Around line 55-73: The deferred resolver currently binds thread_var_ to [0,
block_threads) instead of the actual threadBounds range, breaking nonzero
threadBounds.min cases. Update the constructor and its call from the resolver
setup to accept the complete thread_bounds range, bind analyzer_ using that
range, and validate the computed lower bound against thread_bounds.min.
---
Nitpick comments:
In `@src/backend/common/op/reduce.h`:
- Around line 1121-1127: The attribute key is duplicated as a raw literal and a
local constexpr, allowing producer and consumer names to diverge. In
src/transform/lower_tile_op.cc lines 45-46, move kDeferredScalarAllReduceBarrier
into a shared header alongside the existing tl::attr keys; in
src/backend/common/op/reduce.h lines 1121-1127, replace the inline string with
that shared constant, and update the consumer to reference the same definition.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 2e0adc97-a075-4294-ac40-72a686375a0f
📒 Files selected for processing (3)
src/backend/common/op/reduce.hsrc/transform/lower_tile_op.cctesting/python/language/test_tilelang_language_reduce.py
eeba659 to
2d4bcd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@testing/python/language/test_tilelang_language_reduce.py`:
- Around line 279-287: Update
test_reduce_partial_thread_barrier_rejects_disjoint_groups to escape the dot in
the pytest.raises match pattern, using a raw or equivalent escaped regex so it
matches the literal “threadIdx.x” text emitted by LowerTileOp.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 86384193-9dc5-4940-bffc-735ee7c4ca2f
📒 Files selected for processing (3)
src/backend/common/op/reduce.hsrc/transform/lower_tile_op.cctesting/python/language/test_tilelang_language_reduce.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/common/op/reduce.h
- src/transform/lower_tile_op.cc
2d4bcd7 to
72e192f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testing/python/language/test_tilelang_language_reduce.py (1)
1109-1109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse raw strings for the
match=patterns to clear RUF043.The
.*is intentional here, but Ruff still flags the non-raw literal; making it raw keeps CI clean with no behavior change. Same rule was raised on an earlier commit for anothermatch=pattern in this file.🔧 Proposed fix
- with pytest.raises(Exception, match="logical_width.*positive power of two"): + with pytest.raises(Exception, match=r"logical_width.*positive power of two"):Also applies to: 1137-1137
🤖 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 `@testing/python/language/test_tilelang_language_reduce.py` at line 1109, Update the pytest.raises match patterns at the affected locations in the reduction tests to use raw string literals, preserving the existing regular expressions and exception behavior while clearing RUF043.Source: Linters/SAST tools
🧹 Nitpick comments (4)
src/config.h (1)
52-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
kMaxNamedBarrierinstead of the literal15.This PR introduces
tl::kMaxNamedBarrierinsrc/transform/common/thread_sync_types.h; referencing it here keeps the hardware bound in one place.♻️ Proposed change
- if (start < 2 || start > 15) { - LOG(FATAL) << "tl.named_barrier_start must be in [2, 15], got " << start; + if (start < 2 || start > kMaxNamedBarrier) { + LOG(FATAL) << "tl.named_barrier_start must be in [2, " << kMaxNamedBarrier + << "], got " << start; }Requires including
transform/common/thread_sync_types.hhere.🤖 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 `@src/config.h` around lines 52 - 54, Update the start-range validation in the configuration check to compare against tl::kMaxNamedBarrier instead of the literal 15, and include transform/common/thread_sync_types.h so the symbol is available. Keep the existing lower bound and fatal error behavior unchanged.src/backend/common/op/reduce.h (1)
207-238: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffExhaustive enumeration can cost up to ~1M
Substitute+Simplifycalls at compile time.
kMaxCombos = 1 << 20bounds correctness but not compile time; each combination runs a full TIR substitution and simplification. For wide fragment layouts this can dominate lowering. Consider deriving the image analytically (or memoizing/short-circuiting once contiguity and span are established), or lowering the cap to something defensible with a clear error.🤖 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 `@src/backend/common/op/reduce.h` around lines 207 - 238, Replace the exhaustive enumeration in the reduce-layout image analysis with a bounded-cost approach: derive the image analytically or memoize/short-circuit once contiguity and span are established, rather than invoking Substitute and Simplify for every combination. Update the kMaxCombos handling to enforce a compile-time-defensible limit and retain a clear fatal error when the layout cannot be resolved safely.testing/python/transform/test_tilelang_transform_thread_sync.py (1)
821-822: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
requires_cudalooks unnecessary here.This test only runs the
ThreadSyncpass over an IRModule with no target attribute and no device execution, unlike the sibling tests at Lines 767 and 795 which are ungated. Dropping the gate keeps this coverage alive on CPU-only CI. The duplicatedfuncbody withtest_partial_sync_warp_multiple_still_loweredcould also be shared via a small helper.🤖 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 `@testing/python/transform/test_tilelang_transform_thread_sync.py` around lines 821 - 822, Remove the requires_cuda decorator from test_partial_sync_honors_named_barrier_start_attr so the IR-only ThreadSync coverage runs on CPU-only CI. Keep the test’s existing behavior unchanged; optionally factor its duplicated func setup with test_partial_sync_warp_multiple_still_lowered only if a small local helper preserves readability.testing/python/language/test_tilelang_language_reduce.py (1)
592-592: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBarrier-ID regex assumes a literal single space; line 942 in this same file uses
,\s*.If codegen emits
NamedBarrier<64,1>(no space),re.findallreturns[]and these assertions fail for the wrong reason. Align with the tolerant pattern already used intest_finalize_reducer_codegen, and hoist it into one module-level constant.♻️ Proposed change
- ids = [int(value) for value in re.findall(r"NamedBarrier<\d+, (\d+)>", src)] + ids = [int(value) for value in re.findall(_BARRIER_ID_RE, src)]Add near the top of the module:
_BARRIER_ID_RE = r"NamedBarrier<\s*\d+\s*,\s*(\d+)\s*>"Also applies to: 605-605, 610-610
🤖 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 `@testing/python/language/test_tilelang_language_reduce.py` at line 592, Define a module-level _BARRIER_ID_RE using whitespace-tolerant matching for NamedBarrier IDs, then replace the inline regex usages in the affected reduce tests and test_finalize_reducer_codegen with this shared constant. Preserve the existing capture of the barrier ID while accepting codegen output with or without spaces.
🤖 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 `@src/backend/common/op/reduce.h`:
- Around line 223-242: Update the terminal branch of the enumerate lambda so any
substituted thread expression that does not simplify to a constant causes a
compile-time fatal error instead of being omitted from image. Preserve insertion
of valid constant values, and include sufficient context in the diagnostic to
identify the unresolved thread-image expression.
- Around line 1178-1186: Update the batched reduction path around
Impl::MakeBatchAllReduce to set reduce::AllReduceBarrier participants for
partial CTA thread images, matching the scalar reduction handling and using the
actual reducing-thread count. Ensure named-barrier generation consumes this
resolved participant count rather than defaulting to the full thread image,
while preserving existing behavior for full CTAs and unsupported targets.
In `@src/transform/thread_storage_sync.cc`:
- Around line 330-338: Update the barrier-ID validation in the thread storage
synchronization flow to reject barrier_id == 0 in addition to IDs exceeding
kMaxNamedBarrier. Preserve the existing fatal error path and message context,
ensuring the reserved __syncthreads barrier cannot be selected.
---
Outside diff comments:
In `@testing/python/language/test_tilelang_language_reduce.py`:
- Line 1109: Update the pytest.raises match patterns at the affected locations
in the reduction tests to use raw string literals, preserving the existing
regular expressions and exception behavior while clearing RUF043.
---
Nitpick comments:
In `@src/backend/common/op/reduce.h`:
- Around line 207-238: Replace the exhaustive enumeration in the reduce-layout
image analysis with a bounded-cost approach: derive the image analytically or
memoize/short-circuit once contiguity and span are established, rather than
invoking Substitute and Simplify for every combination. Update the kMaxCombos
handling to enforce a compile-time-defensible limit and retain a clear fatal
error when the layout cannot be resolved safely.
In `@src/config.h`:
- Around line 52-54: Update the start-range validation in the configuration
check to compare against tl::kMaxNamedBarrier instead of the literal 15, and
include transform/common/thread_sync_types.h so the symbol is available. Keep
the existing lower bound and fatal error behavior unchanged.
In `@testing/python/language/test_tilelang_language_reduce.py`:
- Line 592: Define a module-level _BARRIER_ID_RE using whitespace-tolerant
matching for NamedBarrier IDs, then replace the inline regex usages in the
affected reduce tests and test_finalize_reducer_codegen with this shared
constant. Preserve the existing capture of the barrier ID while accepting
codegen output with or without spaces.
In `@testing/python/transform/test_tilelang_transform_thread_sync.py`:
- Around line 821-822: Remove the requires_cuda decorator from
test_partial_sync_honors_named_barrier_start_attr so the IR-only ThreadSync
coverage runs on CPU-only CI. Keep the test’s existing behavior unchanged;
optionally factor its duplicated func setup with
test_partial_sync_warp_multiple_still_lowered only if a small local helper
preserves readability.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 65af5ad7-1a54-446e-a422-3d8a9e1e8ed3
📒 Files selected for processing (19)
src/backend/common/op/finalize_reducer.hsrc/backend/common/op/reduce.hsrc/config.hsrc/cuda/op/finalize_reducer.ccsrc/cuda/op/reduce.ccsrc/op/builtin.ccsrc/op/builtin.hsrc/op/operator.hsrc/rocm/op/finalize_reducer.ccsrc/rocm/op/reduce.ccsrc/tl_templates/cuda/reduce.hsrc/transform/common/thread_sync_types.hsrc/transform/lower_tile_op.ccsrc/transform/thread_storage_sync.cctesting/python/language/test_tilelang_language_reduce.pytesting/python/target/test_tilelang_codegen_cutedsl_reduce.pytesting/python/transform/test_tilelang_transform_thread_sync.pytilelang/contrib/cutedsl/reduce.pytilelang/transform/pass_config.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/backend/common/op/reduce.h (1)
1008-1016: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBatch AllReduce path still doesn't resolve partial-participation barriers.
Unlike the scalar path (lines 1199-1208) which calls
ResolveAllReduceBarrierto computepartial/base/participantsfor the actual participating range, this batch path only claims abarrier_idand leavesbarrier.partial=false/participants=0. If a batched reduction can also execute with only part of the CTA participating (partial scalar fragment), the named barrier emitted downstream would use the whole-CTA arrival count while only a subset of threads arrive — the same hang/premature-release hazard this PR fixes for the scalar path.If batched reductions are guaranteed to always cover the whole CTA by construction (e.g., rejected earlier during layout validation), this is safe as-is and worth a short comment noting why; otherwise this path needs the same
ResolveAllReduceBarriertreatment.#!/bin/bash # Check whether the batch AllReduce path can ever see partial (non-whole-CTA) participation, # and whether MakeBatchAllReduce differentiates on barrier.partial like the scalar codegen does. rg -n "MakeBatchAllReduce" -A 20 src/cuda/op/reduce.cc src/rocm/op/reduce.cc src/tl_templates/cuda/reduce.h 2>/dev/null rg -n "barrier.partial|barrier.participants" src/cuda/op src/tl_templates/cuda 2>/dev/null🤖 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 `@src/backend/common/op/reduce.h` around lines 1008 - 1016, Validate whether the batch AllReduce path around MakeBatchAllReduce can receive partial CTA participation; if it can, apply ResolveAllReduceBarrier to the barrier before claiming the named barrier so partial, base, and participants reflect the actual range. If batch reductions are guaranteed to cover the whole CTA by prior validation, preserve the current logic and add a concise comment documenting that invariant.
🤖 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 `@src/backend/common/op/reduce.cc`:
- Around line 119-127: Update the terminal branch of enumerate in the reduction
logic to reject substituted thread-image expressions that do not simplify to
integer constants, rather than silently skipping them. Emit the established
compile-time diagnostic or propagate failure so contiguity and warp-alignment
checks cannot proceed with an incomplete image; preserve insertion of valid
constants into image.
---
Duplicate comments:
In `@src/backend/common/op/reduce.h`:
- Around line 1008-1016: Validate whether the batch AllReduce path around
MakeBatchAllReduce can receive partial CTA participation; if it can, apply
ResolveAllReduceBarrier to the barrier before claiming the named barrier so
partial, base, and participants reflect the actual range. If batch reductions
are guaranteed to cover the whole CTA by prior validation, preserve the current
logic and add a concise comment documenting that invariant.
🪄 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: CHILL
Plan: Pro Plus
Run ID: c947aa1d-e56d-4983-88a8-b50a2e10d18b
📒 Files selected for processing (3)
CMakeLists.txtsrc/backend/common/op/reduce.ccsrc/backend/common/op/reduce.h
598c660 to
1ee8bec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testing/python/language/test_tilelang_language_reduce.py (1)
380-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
subprocess.run(timeout=...)instead of shelling out to thetimeoutbinary.
timeoutis GNU coreutils and is absent on macOS/Windows, where the call raisesFileNotFoundError. Underxfail(strict=True)that failure is indistinguishable from the hang this test is meant to observe, so the test silently reports "expected failure" for the wrong reason. The Python-level timeout is portable and makes the expectation explicit. (Ruff's S603/S607 hits here are benign — all inputs come fromsys.executable/__file__.)♻️ Proposed change
- result = subprocess.run( - ["timeout", "15s", sys.executable, str(Path(__file__).resolve())], - env={**os.environ, "TILELANG_PRE_SM90_PARTIAL_REDUCE_CHILD": "1"}, - capture_output=True, - text=True, - ) - assert result.returncode == 0, ( - f"partial scalar reduction did not complete on pre-SM90 CUDA (exit code {result.returncode}):\n{result.stderr}" - ) + try: + result = subprocess.run( + [sys.executable, str(Path(__file__).resolve())], + env={**os.environ, "TILELANG_PRE_SM90_PARTIAL_REDUCE_CHILD": "1"}, + capture_output=True, + text=True, + timeout=15, + ) + except subprocess.TimeoutExpired: + pytest.fail("partial scalar reduction hung on pre-SM90 CUDA") + assert result.returncode == 0, ( + f"partial scalar reduction did not complete on pre-SM90 CUDA (exit code {result.returncode}):\n{result.stderr}" + )🤖 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 `@testing/python/language/test_tilelang_language_reduce.py` around lines 380 - 389, Replace the external "timeout" wrapper in test_reduce_partial_thread_barrier_pre_sm90_runtime with subprocess.run’s Python-level timeout argument, keeping sys.executable, the resolved test path, environment, and output capture unchanged. Preserve the 15-second timeout behavior while avoiding reliance on the platform-specific timeout binary.Source: Linters/SAST tools
src/backend/common/op/reduce.h (2)
232-243: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEnumeration is authoritative; the
const_int_boundcross-checks can misfire as "internal error".
analyzer.const_int_boundis an over-approximation, so a layout whose exact image is contiguous but narrower than the analyzer's bound (e.g. any thread expression the analyzer cannot tighten) trips Lines 238/243 with an unhelpful "internal error" message instead of the descriptive contiguity/alignment diagnostics. Sinceimageis computed exactly, consider dropping these two equality checks (keeping the earlier ±inf reachability check) or demoting them toDLOG.🤖 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 `@src/backend/common/op/reduce.h` around lines 232 - 243, Remove the ICHECK_EQ validations comparing image_min/image_max with bound->min_value/bound->max_value in the partial scalar AllReduce barrier logic. Treat the exactly enumerated image range as authoritative, retain the existing image_count contiguity check and earlier ±inf reachability validation, and preserve their descriptive diagnostics.
193-227: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCompile-time cost of the enumeration cap.
kMaxCombos = 1 << 20permits up to ~1MSubstitute+analyzer.Simplifycalls perT.reduce, each allocating PrimExpr nodes — that can dominate lowering time for wide fragments. A much smaller cap (e.g. 2^16) would still cover realistic reduce layouts while failing fast on pathological ones; alternatively hoist a singleSubstituteof the constant map per combo without re-running full simplification.🤖 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 `@src/backend/common/op/reduce.h` around lines 193 - 227, Reduce the enumeration limit in the thread-image resolution logic around kMaxCombos from 2^20 to a substantially smaller bound such as 2^16, while preserving the existing fatal error for layouts exceeding the cap. Keep the enumerate callback and its Substitute/analyzer.Simplify behavior unchanged unless a safe single-substitution optimization is already supported.
🤖 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 `@testing/python/language/test_tilelang_language_reduce.py`:
- Around line 392-396: Make the child-process branch containing
_make_partial_reduce_kernel mutually exclusive with the normal
tilelang.testing.main() runner by adding an else around the original __main__
runner or exiting after the child assertions. Ensure child execution runs only
the partial-reduce validation and normal execution still invokes
tilelang.testing.main().
---
Nitpick comments:
In `@src/backend/common/op/reduce.h`:
- Around line 232-243: Remove the ICHECK_EQ validations comparing
image_min/image_max with bound->min_value/bound->max_value in the partial scalar
AllReduce barrier logic. Treat the exactly enumerated image range as
authoritative, retain the existing image_count contiguity check and earlier ±inf
reachability validation, and preserve their descriptive diagnostics.
- Around line 193-227: Reduce the enumeration limit in the thread-image
resolution logic around kMaxCombos from 2^20 to a substantially smaller bound
such as 2^16, while preserving the existing fatal error for layouts exceeding
the cap. Keep the enumerate callback and its Substitute/analyzer.Simplify
behavior unchanged unless a safe single-substitution optimization is already
supported.
In `@testing/python/language/test_tilelang_language_reduce.py`:
- Around line 380-389: Replace the external "timeout" wrapper in
test_reduce_partial_thread_barrier_pre_sm90_runtime with subprocess.run’s
Python-level timeout argument, keeping sys.executable, the resolved test path,
environment, and output capture unchanged. Preserve the 15-second timeout
behavior while avoiding reliance on the platform-specific timeout binary.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 2c511773-b003-4270-b615-68b2c3d17ce1
📒 Files selected for processing (6)
src/backend/common/op/reduce.hsrc/cuda/op/reduce.ccsrc/op/operator.hsrc/rocm/op/reduce.ccsrc/transform/lower_tile_op.cctesting/python/language/test_tilelang_language_reduce.py
8829249 to
857b881
Compare
857b881 to
0f7b2b2
Compare
… allreduce_barriers doc - Inline ResolveAllReduceThreadRange body from reduce.cc into reduce.h, add #include <tvm/ir/with_context.h> for With<ConstraintContext> usage. - Delete src/backend/common/op/reduce.cc (function now in header). - Remove reduce.cc from CMakeLists.txt TILE_LANG_SRCS. - Delete docs/compiler_internals/allreduce_barriers.md. - Remove toctree entry from docs/index.md.
LeiWang1999
left a comment
LeiWang1999
left a comment
There was a problem hiding this comment.
Overall LGTM, but likely we should fix batch reduce as well.
This reverts commit 25ee210.
44762c7 to
16d091e
Compare
T.reducewill produce wrong barrier counts inSM >= 90.This can be reproduced with
Now this issue is fixed by introducing intermediate representation in pass
LowerTileOp.For convince, reduces that use non-uniform registers layouts in a warp (e.g. using registers only in threads that
tx % 2 == 0) or use registers in non-consecutive warps (e.g.tx [0, 32)andtx [64, 92)) will now be rejected by the compiler.Extra tests over
T.reduceare added.2 bugs are observed but not fixed in current PR:
SM < 90, where the default sync method is__syncthreadsand will require all threads in a block to participate inT.reduce.if tx < 32: T.reduce(A) elif tx < 64 T.reduce(B)) callingT.reduceat the same time will interfere each other's barriers as the current code hardcodes barrier 1 and 2 inAllReduce.They are expected to be fixed in other 2 PRs.
Summary
T.reducebarrier counts on SM90+ for reductions involving only part of a scalar fragment.C++ style / lint notes
docs/developer_guide/cpp_style.mdare indicated.