feat(harness): Add PTO backend support and PTOAS on-board tests#278
feat(harness): Add PTO backend support and PTOAS on-board tests#278lyfne123 merged 1 commit intohw-native-sys:mainfrom
Conversation
Summary of ChangesHello @lwDavid, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the test harness by introducing explicit support for the PTO backend, alongside the existing CCE backend. It refactors the code generation process to be configurable by individual test cases, allowing them to specify their target backend. This change is crucial for expanding test coverage to the PTO backend and validating its functionality, as demonstrated by the inclusion of a new test for a vector DAG program using PTOAS optimization. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes introduce dynamic backend type selection in the test harness infrastructure. A new optional Changes
Sequence DiagramsequenceDiagram
participant TestCase as Test Case
participant Runner as Test Runner
participant Generator as ProgramCodeGenerator
participant Compiler as ir.compile()
TestCase->>Runner: get_backend_type()
Runner->>Runner: backend_type = BackendType.PTO (or CCE)
Runner->>Runner: set_backend_type(backend_type)
Runner->>Generator: __init__(strategy, backend_type)
Generator->>Generator: self.backend_type = backend_type
Generator->>Compiler: generate() calls ir.compile(backend_type=self.backend_type)
Compiler->>Compiler: Compile with selected backend
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively adds support for the PTO backend in the test harness, making it more flexible. The changes are well-implemented, propagating the backend_type from the test case down to the code generator while maintaining backward compatibility by defaulting to the CCE backend. The new test test_vector_dag_pto.py is a great addition for end-to-end validation of this new path. I have one suggestion in the new test file to improve type safety, aligning with best practices for passing primitive-like types to internal bindings.
| d: pl.Tensor[[128, 128], pl.FP32] = self.kernel_add_scalar(c, 1.0) # type: ignore[reportArgumentType] | ||
| e: pl.Tensor[[128, 128], pl.FP32] = self.kernel_add_scalar(c, 2.0) # type: ignore[reportArgumentType] |
There was a problem hiding this comment.
Using type: ignore[reportArgumentType] hides a potential type mismatch. Instead of passing a raw float and suppressing the type checker, you should use pl.const() to explicitly create a DSL scalar constant. This resolves the type error, removes the need for # type: ignore, and makes the code more robust and readable. This aligns with the guideline that when passing primitive-like types to internal bindings, they should be explicitly converted to the expected internal representation (e.g., Scalar or Expr) to match the expected C++ types.
| d: pl.Tensor[[128, 128], pl.FP32] = self.kernel_add_scalar(c, 1.0) # type: ignore[reportArgumentType] | |
| e: pl.Tensor[[128, 128], pl.FP32] = self.kernel_add_scalar(c, 2.0) # type: ignore[reportArgumentType] | |
| d: pl.Tensor[[128, 128], pl.FP32] = self.kernel_add_scalar(c, pl.const(1.0)) | |
| e: pl.Tensor[[128, 128], pl.FP32] = self.kernel_add_scalar(c, pl.const(2.0)) |
References
- When passing a sequence of IntLike (int | Scalar | Expr) to C++ bindings, ensure any Scalar objects are unwrapped to Expr objects to match the expected C++ types.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/st/harness/adapters/program_generator.py (1)
29-29: Consider updating docstrings to reflect multi-backend support.The class docstring says "Generates CCE C++ kernel..." but now supports both CCE and PTO backends. Similarly, the
generate()method docstring at line 130 mentions "CCE C++ kernel files."📝 Suggested docstring updates
class ProgramCodeGenerator: - """Generates CCE C++ kernel and orchestration code from PyPTO Programs. + """Generates C++ kernel and orchestration code from PyPTO Programs. Pipeline: ir.Program -> ir.compile() -> C++ source filesAnd at line 130:
def generate( ... ) -> dict[str, Any]: - """Generate CCE C++ kernel files from a PyPTO Program. + """Generate C++ kernel files from a PyPTO Program.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/st/harness/adapters/program_generator.py` at line 29, Update the class docstring and the generate() method docstring in program_generator.py to reflect multi-backend support (both CCE and PTO) instead of only "CCE C++ kernel"; specifically edit the top-level class docstring that currently begins "Generates CCE C++ kernel and orchestration code from PyPTO Programs." and the docstring inside the generate() method (around line 130) that mentions "CCE C++ kernel files" so both describe that the generator produces backend-specific code for CCE and PTO (or a generic "CCE/PTO" phrasing) and note any backend selection behavior handled by the generate() method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/st/harness/adapters/program_generator.py`:
- Line 29: Update the class docstring and the generate() method docstring in
program_generator.py to reflect multi-backend support (both CCE and PTO) instead
of only "CCE C++ kernel"; specifically edit the top-level class docstring that
currently begins "Generates CCE C++ kernel and orchestration code from PyPTO
Programs." and the docstring inside the generate() method (around line 130) that
mentions "CCE C++ kernel files" so both describe that the generator produces
backend-specific code for CCE and PTO (or a generic "CCE/PTO" phrasing) and note
any backend selection behavior handled by the generate() method.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/st/harness/adapters/program_generator.pytests/st/harness/core/harness.pytests/st/harness/core/test_runner.pytests/st/runtime/test_vector_dag_pto.py
lwDavid
force-pushed
the
dim
branch
3 times, most recently
from
4508a21 to
e743431
Compare
lwDavid
changed the title
|
@Hzfengsy Request review. |
2 participants
feat(harness): Add PTO backend support and PTOAS on-board tests
Enable test harness to use PTO backend (via ptoas compiler) in addition
to the default CCE backend.
Harness changes:
Runtime tests:
PTOAS strategy to run with CCE backend (sync not inserted → wrong results)
CI: