feat(vpto): add A2/A3 gather lowering#971
Conversation
![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 implements tile-level gather lowering (pto.tgather and pto.tgatherb) for the A2/A3 vpto backend, introducing new flat UB-pointer gather ops (pto.ub.vgather and pto.ub.vgatherb) and their lowerings to LLVM intrinsics with packed config words. It also adds synchronization macro models, extensive end-to-end tests, and lit verification tests. The review feedback suggests several improvements: emitting a compilation error instead of silently skipping lowering when srcAddr cannot be resolved, using safer defensive checks for validCol, explicitly casting mixed signed/unsigned types to prevent compiler warnings, and using castOp->getOperand(0) to avoid potential C++ name hiding issues.
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.
| Value srcAddr; | ||
| if (auto *defOp = op.getSrc().getDefiningOp()) | ||
| if (isa<pto::CastPtrOp>(defOp)) | ||
| srcAddr = defOp->getOperand(0); | ||
| if (!srcAddr) | ||
| continue; |
There was a problem hiding this comment.
Silently skipping the lowering when srcAddr cannot be resolved from a CastPtrOp will leave the pto.tgather op unlowered in the IR, leading to downstream compiler crashes or failures that are difficult to diagnose. Instead of silently continuing, emit a clear compilation error and signal pass failure.
| Value srcAddr; | |
| if (auto *defOp = op.getSrc().getDefiningOp()) | |
| if (isa<pto::CastPtrOp>(defOp)) | |
| srcAddr = defOp->getOperand(0); | |
| if (!srcAddr) | |
| continue; | |
| Value srcAddr; | |
| if (auto *defOp = op.getSrc().getDefiningOp()) | |
| if (isa<pto::CastPtrOp>(defOp)) | |
| srcAddr = defOp->getOperand(0); | |
| if (!srcAddr) { | |
| op.emitOpError("expects src operand to be defined by pto.castptr"); | |
| signalPassFailure(); | |
| return; | |
| } |
| unsigned epr = info->elementsPerRepeat; | ||
| int64_t validRow = info->vRows; | ||
| int64_t validCol = info->vCols; | ||
| if (bse == 0 || epr == 0 || validCol == 0) |
There was a problem hiding this comment.
If validCol is negative, it will pass the validCol == 0 check and can cause division-by-zero or negative repeat counts later. Use validCol <= 0 for safer defensive programming.
| if (bse == 0 || epr == 0 || validCol == 0) | |
| if (bse == 0 || epr == 0 || validCol <= 0) |
|
|
||
| // Compact offset tile layout: one i32 address per 32B output block, | ||
| // padded to 8 entries because even a tail repeat reads 8 addresses. | ||
| int64_t blocksPerRow = (validCol + bse - 1) / bse; |
There was a problem hiding this comment.
Mixing signed int64_t (validCol) and unsigned unsigned (bse) in arithmetic operations can trigger compiler warnings (e.g., -Wsign-compare or -Wconversion) and potentially lead to unexpected implicit promotions. Explicitly cast bse to int64_t.
| int64_t blocksPerRow = (validCol + bse - 1) / bse; | |
| int64_t blocksPerRow = (validCol + static_cast<int64_t>(bse) - 1) / static_cast<int64_t>(bse); |
| Value srcAddr; | ||
| if (auto *defOp = op.getSrc().getDefiningOp()) { | ||
| if (auto castOp = dyn_cast<pto::CastPtrOp>(defOp)) | ||
| srcAddr = castOp.getOperand(); |
There was a problem hiding this comment.
Calling castOp.getOperand() relies on a TableGen-generated getter if the operand is named operand. However, this can conflict with or hide the standard OpState::getOperand(unsigned) method in C++ due to name hiding rules. Using castOp->getOperand(0) is safer, more standard in MLIR, and avoids any potential C++ name hiding or compilation issues across different compiler/MLIR versions.
| srcAddr = castOp.getOperand(); | |
| srcAddr = castOp->getOperand(0); |
1 participant
Summary
pto.tgatherbthroughpto.ub.vgatherbto packedllvm.hivm.VGATHERB.b16/.b32calls.pto.tgatherindex form throughvmuls + vgather, preserving count-mode mask lifetime and chunking wide rows.tgatherbas a 32-byte block gather with one compact i32 block address per output block, padded to eight entries per repeat.tgatherbsupport merged in Add TGATHERB #932 without duplicating its wrapper or namespace registration.Notes
tgatherbis a 32-byte block gather, not scalar element gather.tgatherindex form andvgather.mgatherremains out of scope because it is A5-only.Validation
ptoasbuild: passed.Covered NPU shapes:
tgatherbf32:1x8,1x64,1x96,2x64,1x128,4x64tgatherbf16:1x16,1x128,1x192tgatherf32/f16:1x64,2x64,2x128