Move SASS diff description to a SKILL#9767
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new skill file for SASS/PTX diff checks, links it from the Claude skills path, and removes the old SASS Diffs section from AGENTS.md. ChangesSASS diff skill relocation
Assessment against linked issues
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80db501d-a267-462f-ad53-e71ae893129b
📒 Files selected for processing (3)
.agent/skills/sass-diff/SKILL.md.claude/skills/sass-diff/SKILL.mdAGENTS.md
💤 Files with no reviewable changes (1)
- AGENTS.md
| ## Inputs to establish | ||
|
|
||
| * Compiled binary under test | ||
| * The CUDA SM architectures to compile for. Try to detect this from the code and offer the user a list of suggestions. | ||
| The user must conform or provide this list. | ||
| * Baseline disassembly (from the previous commit/branch or the current commit without the changes in the working copy). | ||
| * Comparison disassembly (form the current commit/branch or the current commit with the changes in the working copy). | ||
| * By default, prefer `cuobjdump -sass` to inspect SASS changes. | ||
| Use `cuobjdump -ptx` if the request is to check for PTX changes instead. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
important: This still starts from prebuilt disassemblies. The issue requires the skill to take baseline and modified CUDA sources plus a target SM, compile both with the same nvcc flags, and then diff the cuobjdump output; otherwise agents can compare non-equivalent artifacts.
🧰 Tools
🪛 LanguageTool
[grammar] ~21-~21: Ensure spelling is correct
Context: ...orking copy). * Comparison disassembly (form the current commit/branch or the curren...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
| ## Reporting | ||
|
|
||
| * If any non-trivial change was detected, the top 5 regions where a non-trivial change was detected, | ||
| including the name of the kernel they appeared in. | ||
| * A short summary of the diff type (opcode change, memory access size change, size delta, control-flow, etc.). | ||
| * Explicitly state if only noise was detected after normalization. | ||
| * If you are not sure if the differences are impactful, show it and ask the user for guidance. | ||
| * Keep the disassembly dumps available for reference and show the command to the user to generate a diff. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
important: The reporting section is missing the required performance regressions. Add explicit register-count and occupancy deltas to the summary so those regressions are surfaced instead of normalized away.
f956a5f to
9a3165a
Compare
| Apply these transforms to both baseline and candidate listings before diffing. | ||
| Write the normalized listings to separate files. | ||
|
|
||
| * Remove addresses/offsets/hex location prefixes. | ||
| * Remove build IDs, timestamps, absolute paths, temp directories, and compiler banners. | ||
| * Normalize whitespace and alignment to single spaces. | ||
| * Remove empty lines and purely comment lines. |
There was a problem hiding this comment.
IME these skills work much better if the agent has a set of scripts they can run that do these highly deterministic things for them. You should ask the bot to generate such a script for you and check it in with the skill.
There was a problem hiding this comment.
+1. Otherwise, the agent needs to write the script every time
| * The CUDA SM architectures to compile for. Try to detect this from the code and offer the user a list of suggestions. | ||
| The user must confirm or provide this list. |
There was a problem hiding this comment.
I would note that the bot should also check the compile_commands.json to get a definitive answer for this. The architectures will be listed explicitly in the compiler flags.
| ## Normalization rules (strip known noise) | ||
|
|
||
| Apply these transforms to both baseline and candidate listings before diffing. | ||
| Write the normalized listings to separate files. | ||
|
|
||
| * Remove addresses/offsets/hex location prefixes. | ||
| * Remove build IDs, timestamps, absolute paths, temp directories, and compiler banners. | ||
| * Normalize whitespace and alignment to single spaces. | ||
| * Remove empty lines and purely comment lines. | ||
|
|
||
| ## Comparison rules (what matters) | ||
|
|
||
| Ignore as trivial: | ||
|
|
||
| * Register renaming with identical instruction sequence and operands. | ||
| * Pure label renumbering or reordering of identical basic blocks. | ||
| * Formatting-only differences or reordered symbol tables. |
There was a problem hiding this comment.
Can we add a checked-in normalization script so agents do not rewrite this logic each time?
I use something like this (also agent-generated but it works for me so far).
| * The CUDA SM architectures to compile for. Try to detect this from the code and offer the user a list of suggestions. | ||
| The user must confirm or provide this list. |
There was a problem hiding this comment.
| * The CUDA SM architectures to compile for. Try to detect this from the code and offer the user a list of suggestions. | |
| The user must confirm or provide this list. | |
| * The CUDA SM architectures to compile for. Try to detect this from the code and offer the user a list of suggestions. | |
| The user must confirm or provide this list. | |
| * Prefer `compile_commands.json` when available to recover the actual `nvcc` flags and `--generate-code` / `-arch` settings. |
|
|
||
| * If any non-trivial change was detected, the top 5 regions where a non-trivial change was detected, | ||
| including the name of the kernel they appeared in. | ||
| * A short summary of the diff type (opcode change, memory access size change, size delta, control-flow, etc.). |
There was a problem hiding this comment.
| * A short summary of the diff type (opcode change, memory access size change, size delta, control-flow, etc.). | |
| * A short summary of the diff type, including opcode changes, memory access size/cache policy changes, control-flow changes, register-count changes, spills/local memory, shared memory, and occupancy-relevant resource deltas. |
Fixes: #7829