Skip to content

[Tests] Check WebGPU volatile allreduce annotation structurally#19740

Merged
tlopex merged 1 commit into
apache:mainfrom
tlopex:fix-s-tir-webgpu-volatile
Jun 12, 2026
Merged

[Tests] Check WebGPU volatile allreduce annotation structurally#19740
tlopex merged 1 commit into
apache:mainfrom
tlopex:fix-s-tir-webgpu-volatile

Conversation

@tlopex

@tlopex tlopex commented Jun 11, 2026

Copy link
Copy Markdown
Member

This pr updates the WebGPU multi-warp allreduce test to check the generated tirx.volatile allocation annotation structurally instead of matching the exact TVMScript printer output.

The test is intended to verify that LowerThreadAllreduce marks the generated shared allocation as volatile. It previously checked for the exact string:

"tirx.volatile": T.bool(True)

However, the current printer emits the same annotation as:

annotations={"tirx.volatile": True}

The transform behavior is unchanged; only the printer spelling differs. This patch walks the generated TIRX body and checks for an AllocBuffer with tirx.volatile=True, which matches the actual semantic requirement of the test without depending on bool literal formatting.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request replaces a fragile string-based assertion in the thread all-reduce lower transform tests with a helper function, _has_volatile_alloc_buffer, which programmatically inspects the AST of a TVM module for volatile buffer allocations. The review feedback correctly points out that using identity comparison (is True) on TVM annotations can fail because TVM map lookups return TVM object wrappers rather than Python's built-in True singleton, and suggests using bool(...) instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

def visit(node):
nonlocal has_volatile_alloc
if isinstance(node, tvm.tirx.AllocBuffer) and "tirx.volatile" in node.annotations:
has_volatile_alloc = has_volatile_alloc or node.annotations["tirx.volatile"] is True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using is True to check the value of a TVM annotation can lead to failures. TVM map lookups typically return TVM object wrappers (such as tvm.tir.IntImm or tvm.runtime.Bool) rather than Python's built-in True singleton, so identity comparison (is) will evaluate to False. Using bool(...) is more robust and correctly evaluates the truthiness of the TVM object.

Suggested change
has_volatile_alloc = has_volatile_alloc or node.annotations["tirx.volatile"] is True
has_volatile_alloc = has_volatile_alloc or bool(node.annotations["tirx.volatile"])

@tlopex tlopex merged commit 30bf568 into apache:main Jun 12, 2026
10 checks passed
MasterJH5574 pushed a commit to MasterJH5574/tvm that referenced this pull request Jun 15, 2026
…he#19740)

This pr updates the WebGPU multi-warp allreduce test to check the
generated `tirx.volatile` allocation annotation structurally instead of
matching the exact TVMScript printer output.

The test is intended to verify that `LowerThreadAllreduce` marks the
generated shared allocation as volatile. It previously checked for the
exact string:

```python
"tirx.volatile": T.bool(True)
```

However, the current printer emits the same annotation as:

```python
annotations={"tirx.volatile": True}
```

The transform behavior is unchanged; only the printer spelling differs.
This patch walks the generated TIRX body and checks for an `AllocBuffer`
with `tirx.volatile=True`, which matches the actual semantic requirement
of the test without depending on bool literal formatting.

(cherry picked from commit 30bf568)
MasterJH5574 pushed a commit to MasterJH5574/tvm that referenced this pull request Jun 15, 2026
…he#19740)

This pr updates the WebGPU multi-warp allreduce test to check the
generated `tirx.volatile` allocation annotation structurally instead of
matching the exact TVMScript printer output.

The test is intended to verify that `LowerThreadAllreduce` marks the
generated shared allocation as volatile. It previously checked for the
exact string:

```python
"tirx.volatile": T.bool(True)
```

However, the current printer emits the same annotation as:

```python
annotations={"tirx.volatile": True}
```

The transform behavior is unchanged; only the printer spelling differs.
This patch walks the generated TIRX body and checks for an `AllocBuffer`
with `tirx.volatile=True`, which matches the actual semantic requirement
of the test without depending on bool literal formatting.

(cherry picked from commit 30bf568)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants