feat(codegen): Add skip_ptoas option to PTO backend code generation#280
feat(codegen): Add skip_ptoas option to PTO backend code generation#280lyfne123 merged 1 commit intohw-native-sys:mainfrom
Conversation
Summary of ChangesHello, 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 PTO backend code generation by introducing a new Highlights
Changelog
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:
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 introduces a skip_ptoas option to the compile and generate functions, allowing users to bypass the ptoas compilation step and directly emit raw MLIR files. The implementation is clean and correctly handles the new flag, including skipping the generation of kernel_config.py when ptoas is not invoked. I've added one suggestion to improve the completeness of the new test case, aligning with best practices for specific negative assertions in tests.
| assert len(kernel_keys) > 0, "Expected at least one kernel file" | ||
| for key in kernel_keys: | ||
| assert key.endswith(".pto"), f"Expected .pto extension, got: {key}" | ||
| assert not key.endswith(".cpp"), f"Unexpected .cpp extension: {key}" |
There was a problem hiding this comment.
This test correctly verifies that .pto files are generated when skip_ptoas=True. However, it's missing a check to ensure that kernel_config.py is not generated in this case, as described in the PR description and implemented in pto_codegen.py. Please add the following assertion after this block:
assert "kernel_config.py" not in result, "kernel_config.py should not be generated when skip_ptoas=True"References
- When writing negative assertions in tests, be specific about the pattern being excluded to avoid ambiguity and potential false positives. The suggested assertion
assert "kernel_config.py" not in resultadheres to this by being specific about the file name being excluded.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/ut/codegen/test_pto_codegen.py (1)
531-556: Consider adding an assertion thatkernel_config.pyis absent.The test verifies
.ptofiles are generated but doesn't verify the expected side effect thatkernel_config.pyis skipped whenskip_ptoas=True.🧪 Suggested additional assertion
for key in kernel_keys: assert key.endswith(".pto"), f"Expected .pto extension, got: {key}" assert not key.endswith(".cpp"), f"Unexpected .cpp extension: {key}" + + # Verify kernel_config.py is not generated when skip_ptoas=True + assert "kernel_config.py" not in result, "kernel_config.py should not be generated with skip_ptoas=True"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ut/codegen/test_pto_codegen.py` around lines 531 - 556, Add an assertion after calling generate(transformed_program, ..., skip_ptoas=True) in TestGenerateSkipPtoas that verifies the generated result does not include the kernel_config.py artifact; specifically check that "kernel_config.py" is not present in the keys of the result returned by generate (this complements the existing checks on kernel file extensions and ensures skip_ptoas=True also suppresses kernel_config.py generation).python/pypto/ir/compile.py (1)
86-96: Consider validatingskip_ptoasusage with non-PTO backends.If a user passes
skip_ptoas=TruewithBackendType.CCE, it's silently ignored. While the docstring documents this is "PTO backend only", a warning could help users catch misconfigurations.
⚠️ Optional: Add a warning for skip_ptoas with non-PTO backends+import warnings + def compile( ... ) -> str: ... + if skip_ptoas and backend_type != BackendType.PTO: + warnings.warn( + "skip_ptoas is only supported for PTO backend; ignoring for {backend_type.name}", + stacklevel=2, + ) + if backend_type == BackendType.PTO:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/pypto/ir/compile.py` around lines 86 - 96, The code silently ignores skip_ptoas when backend_type != BackendType.PTO; update the compile path (the block using backend_type, BackendType, skip_ptoas, generate, and CCECodegen in python/pypto/ir/compile.py) to emit a warning when skip_ptoas is True but backend_type is not BackendType.PTO (use the project's logger or warnings.warn) before proceeding to CCECodegen(). Keep behavior unchanged otherwise and include a clear message stating that skip_ptoas is PTO-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/pypto/ir/compile.py`:
- Around line 86-96: The code silently ignores skip_ptoas when backend_type !=
BackendType.PTO; update the compile path (the block using backend_type,
BackendType, skip_ptoas, generate, and CCECodegen in python/pypto/ir/compile.py)
to emit a warning when skip_ptoas is True but backend_type is not
BackendType.PTO (use the project's logger or warnings.warn) before proceeding to
CCECodegen(). Keep behavior unchanged otherwise and include a clear message
stating that skip_ptoas is PTO-only.
In `@tests/ut/codegen/test_pto_codegen.py`:
- Around line 531-556: Add an assertion after calling
generate(transformed_program, ..., skip_ptoas=True) in TestGenerateSkipPtoas
that verifies the generated result does not include the kernel_config.py
artifact; specifically check that "kernel_config.py" is not present in the keys
of the result returned by generate (this complements the existing checks on
kernel file extensions and ensures skip_ptoas=True also suppresses
kernel_config.py generation).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
python/pypto/ir/compile.pypython/pypto/ir/pto_codegen.pytests/ut/codegen/test_pto_codegen.py
Add skip_ptoas parameter to generate() and compile() to bypass the ptoas compilation step and emit raw MLIR (.pto) files directly. Orchestration code is still generated; kernel_config.py is skipped when ptoas is not invoked.
1 participant
Add skip_ptoas parameter to generate() and compile() to bypass the ptoas compilation step and emit raw MLIR (.pto) files directly. Orchestration code is still generated; kernel_config.py is skipped when ptoas is not invoked.