patches on 1.70 - #356
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
Changes are minimal, consistent with existing versioning (v1.70.00) and should improve CI reliability without altering runtime logic.
Pull request overview
Aligns the client-reported patch version with the current TransferBench release (v1.70.00) and adjusts the CodeQL CI setup to install git more reliably in the container environment.
Changes:
- Updates
CLIENT_VERSIONto"00"so the client reportsv1.70.00(matching the changelog/versioning). - Simplifies CodeQL workflow OS setup by removing the git PPA steps and installing
gitviaapt-getwith noninteractive settings.
File summaries
| File | Description |
|---|---|
src/client/EnvVars.hpp |
Sets client patch version to 00 to match TransferBench::VERSION/release versioning (1.70.00). |
.github/workflows/codeql.yml |
Updates apt install steps to install git noninteractively and removes PPA-related commands. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Pingpong post-prep currently continues when a pong-half resource is missing, which can leave a null localFlagMem and later cause a hang/crash in the pingpong GPU kernel.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| auto it = pongByTransferIdx.find(pingRss->transferIdx); | ||
| if (it == pongByTransferIdx.end()) continue; | ||
| TransferResources* pongRss = it->second; |
gilbertlee-amd
left a comment
gilbertlee-amd
left a comment
There was a problem hiding this comment.
Remove ping pong latency test
There was a problem hiding this comment.
🟡 Changes recommended
The new pingpong GPU kernel uses 1-byte atomic store builtins even under NVCC despite CUDA lacking 1-byte atomics, risking incorrect synchronization or hangs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| // TODO: replace with hip_atomic_store | ||
| if (!useSrcMem) { | ||
| if (isPing) { | ||
| __atomic_store_n((uint8_t*)remoteFlag, val, __ATOMIC_RELEASE); | ||
| GpuWait(localFlag, val); | ||
| } else { | ||
| GpuWait(localFlag, val); | ||
| __atomic_store_n((uint8_t*)remoteFlag, val, __ATOMIC_RELEASE); | ||
| } | ||
| } else{ | ||
| uint8_t* const srcPtr = val ? srcVal1 : srcVal0; | ||
| if (isPing) { | ||
| __atomic_store((uint8_t*)remoteFlag, srcPtr, __ATOMIC_RELEASE); | ||
| GpuWait(localFlag, val); | ||
| } else { | ||
| GpuWait(localFlag, val); | ||
| __atomic_store((uint8_t*)remoteFlag, srcPtr, __ATOMIC_RELEASE); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
Pingpong transfer parsing currently truncates multi-device SRC/DST tokens silently, and the pingpong executor documentation is internally inconsistent with enforced validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/header/TransferBench.hpp:201
- Typo in the Transfer docstring: "desintations" should be "destinations".
src/header/TransferBench.hpp:217 - The pingpong Transfer docs say ping/pong executors can be "any ExeType" and even give a DMA example, but TransfersHaveErrors() currently rejects non-GFX executors for pingpong. Update the comment to reflect the current restriction to avoid misleading users.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| auto singleMemOrNull = [](vector<MemDevice> const& mems) { | ||
| if (mems.empty()) return MemDevice{MEM_NULL, 0, 0}; | ||
| return mems[0]; | ||
| }; | ||
|
|
||
| Transfer t; | ||
| t.numLaps = numLaps; | ||
| // Pingpong exchanges a 1-byte flag per lap; numBytes only has to be a non-zero | ||
| // multiple of 4 large enough for the two seed flag values, and is not reported as traffic | ||
| t.numBytes = 8; | ||
| t.numSubExecs = 1; | ||
| t.srcs = {singleMemOrNull(pingHalf.srcs), singleMemOrNull(pongHalf.srcs)}; | ||
| t.dsts = {singleMemOrNull(pingHalf.dsts), singleMemOrNull(pongHalf.dsts)}; |
Motivation
Missed minor version in client in previous merge.
Also updating CI install requirements to fix failed CI test.