Feature/a2a3 llvm#908
Conversation
Full binary ops coverage
Codex Review该评论由 review 机器人自动更新。
SummaryReview failed at stage Findings未生成结构化 findings，因为 review 过程提前失败。 Log Tail |
![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 a direct pointer-based lowering pipeline for A3 (dav-c220-vec) targets, introducing the LowerPTOToUBufOps pass and corresponding LLVM lowering patterns. The code review identified several critical and high-severity issues that must be addressed: a syntax/compilation error in VPTOLLVMEmitter.cpp due to a leftover lambda block; multiple checks in ptoas.cpp and LowerPTOToUBufOps.cpp that restrict the pipeline to "a3" and break "a2" support; an overly restrictive check in VPTOLLVMEmitter.cpp that excludes "dav-c220-cube" and triggers invalid A5 DMA lowering; redundant dead loop generation when headRepeats is zero; and a typo in a macro name in VPTO.cpp.
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.
| target.addLegalOp<ModuleOp>(); | ||
| target.addDynamicallyLegalOp<func::FuncOp>([&](func::FuncOp op) { | ||
| target.addLegalOp<pto::AddPtrOp>();([&](func::FuncOp op) { | ||
| return typeConverter.isSignatureLegal(op.getFunctionType()) && | ||
| typeConverter.isLegal(&op.getBody()); | ||
| }); |
There was a problem hiding this comment.
There is a critical syntax/compilation error on line 11067. The statement target.addLegalOp<pto::AddPtrOp>(); ends with a semicolon, but is immediately followed by a lambda block ([&](func::FuncOp op) { ... }); which was left over from the previous addDynamicallyLegalOp call. This will cause a compilation failure.\n\nPlease restore the addDynamicallyLegalOp<func::FuncOp> call properly alongside the new addLegalOp<pto::AddPtrOp>() call.
target.addLegalOp<ModuleOp>();\n target.addLegalOp<pto::AddPtrOp>();\n target.addDynamicallyLegalOp<func::FuncOp>([&](func::FuncOp op) {\n return typeConverter.isSignatureLegal(op.getFunctionType()) &&\n typeConverter.isLegal(&op.getBody());\n });| auto &kernelModulePM = pm.nest<ModuleOp>(); | ||
| auto moduleArchAttr = | ||
| module->getAttrOfType<mlir::StringAttr>("pto.target_arch"); | ||
| const bool isA3 = moduleArchAttr && moduleArchAttr.getValue() == "a3"; |
There was a problem hiding this comment.
The check isA3 only matches "a3". Since A2 and A3 share the same VPTO lowering pipeline (as documented in the design specs), this check should also include "a2". Otherwise, targeting "a2" will incorrectly fall back to the A5 ExpandTileOp path.
| const bool isA3 = moduleArchAttr && moduleArchAttr.getValue() == "a3"; | |
| const bool isA3 = moduleArchAttr && (moduleArchAttr.getValue() == "a2" || moduleArchAttr.getValue() == "a3"); |
| std::string arch = ptoTargetArch; | ||
| for (char &c : arch) | ||
| c = static_cast<char>(std::tolower(static_cast<unsigned char>(c))); | ||
| options.march = (arch == "a3") ? "dav-c220-vec" : "dav-c310-vec"; |
There was a problem hiding this comment.
The target architecture selection on line 2591 defaults to "dav-c310-vec" (the A5 target) if the architecture is not exactly "a3". If the user targets "a2", this will incorrectly select the A5 target instead of "dav-c220-vec".\n\nUpdate the ternary operator to map both "a2" and "a3" to "dav-c220-vec".
| options.march = (arch == "a3") ? "dav-c220-vec" : "dav-c310-vec"; | |
| options.march = (arch == "a2" || arch == "a3") ? "dav-c220-vec" : "dav-c310-vec"; |
| const bool enableLocalTilePlanning = | ||
| ptoTargetArch != "a3" || effectiveBackend == PTOBackend::VPTO; |
There was a problem hiding this comment.
The check ptoTargetArch != "a3" will evaluate to true when targeting "a2", which incorrectly enables local tile planning unconditionally for "a2" even if the backend is not VPTO.\n\nSince A2 and A3 share the same pipeline, they should use the same condition.
const bool isA2A3 = (ptoTargetArch == "a2" || ptoTargetArch == "a3");\n const bool enableLocalTilePlanning =\n !isA2A3 || effectiveBackend == PTOBackend::VPTO;| auto archAttr = mod->getAttrOfType<StringAttr>("pto.target_arch"); | ||
| if (!archAttr || archAttr.getValue() != "a3") | ||
| return; |
There was a problem hiding this comment.
The LowerPTOToUBufOpsPass currently returns early if the target architecture is not exactly "a3". This prevents the pass from running when targeting "a2", which shares the same lowering pipeline.\n\nUpdate the check to allow both "a2" and "a3".
auto archAttr = mod->getAttrOfType<StringAttr>("pto.target_arch");\n if (!archAttr || (archAttr.getValue() != "a2" && archAttr.getValue() != "a3"))\n return;| bool useA3NonPadded = (march == "dav-c220-vec") && isGmUb && !hasPadding; | ||
| bool useA3UbGm = (march == "dav-c220-vec") && !isGmUb; | ||
| bool useSingleConfig = useA3NonPadded || useA3UbGm; |
There was a problem hiding this comment.
The check march == "dav-c220-vec" is too restrictive because it excludes the A2/A3 cube target "dav-c220-cube". Both vector and cube targets on A2/A3 use the same single-config V220 DMA engine. If "dav-c220-cube" is targeted, this will incorrectly fall back to the A5 dual-config DMA lowering, which is invalid for A2/A3.\n\nUpdate the check to cover any C220 target (e.g., by checking if march starts with "dav-c220" or explicitly checking both vector and cube variants).
bool isC220 = (march == "dav-c220-vec" || march == "dav-c220-cube");\n bool useA3NonPadded = isC220 && isGmUb && !hasPadding;\n bool useA3UbGm = isC220 && !isGmUb;\n bool useSingleConfig = useA3NonPadded || useA3UbGm;| auto forOp = b.create<scf::ForOp>(loc, idxc0(loc, b), | ||
| idxc(headRepeats, loc, b), idxc1(loc, b)); | ||
| b.setInsertionPointToStart(forOp.getBody()); | ||
| Value iv = forOp.getInductionVar(); | ||
| Value off = b.create<arith::MulIOp>(loc, iv, idxc(epr, loc, b)).getResult(); | ||
| Value rd = addPtr(loc, b, dst, ptrTy, off); | ||
| Value r0 = addPtr(loc, b, s0, ptrTy, off); | ||
| Value r1 = addPtr(loc, b, s1, ptrTy, off); | ||
| b.create<pto::UBSetMaskCountOp>(loc); | ||
| b.create<pto::UBSetMaskOp>(loc, i64c(epr, loc, b), i64c0(loc, b)); | ||
| emitUBBinOp<UBop>(loc, b, rd, r0, r1, i64c1(loc, b), i64c8(loc, b)); | ||
| b.create<pto::UBSetMaskNormOp>(loc); | ||
| b.setInsertionPointAfter(forOp); |
There was a problem hiding this comment.
When headRepeats is 0 (which happens when totalV < epr), b.create<scf::ForOp> is still called to generate a loop from 0 to 0. While functionally a no-op, generating dead loops complicates downstream optimization passes and produces redundant IR.\n\nSince headRepeats is a compile-time constant, wrap the loop generation in a C++ conditional check if (headRepeats > 0).
if (headRepeats > 0) {\n auto forOp = b.create<scf::ForOp>(loc, idxc0(loc, b),\n idxc(headRepeats, loc, b), idxc1(loc, b));\n b.setInsertionPointToStart(forOp.getBody());\n Value iv = forOp.getInductionVar();\n Value off = b.create<arith::MulIOp>(loc, iv, idxc(epr, loc, b)).getResult();\n Value rd = addPtr(loc, b, dst, ptrTy, off);\n Value r0 = addPtr(loc, b, s0, ptrTy, off);\n Value r1 = addPtr(loc, b, s1, ptrTy, off);\n b.create<pto::UBSetMaskCountOp>(loc);\n b.create<pto::UBSetMaskOp>(loc, i64c(epr, loc, b), i64c0(loc, b));\n emitUBBinOp<UBop>(loc, b, rd, r0, r1, i64c1(loc, b), i64c8(loc, b));\n b.create<pto::UBSetMaskNormOp>(loc);\n b.setInsertionPointAfter(forOp);\n }| return success(); | ||
| } | ||
|
|
||
| #define PTO_DEFINE_UB_UNARY_VERITY_AND_EFFECTS(OpName) \ |
There was a problem hiding this comment.
There is a typo in the macro name: VERITY instead of VERIFY. Please rename it to PTO_DEFINE_UB_UNARY_VERIFY_AND_EFFECTS for consistency and readability.
| #define PTO_DEFINE_UB_UNARY_VERITY_AND_EFFECTS(OpName) \ | |
| #define PTO_DEFINE_UB_UNARY_VERIFY_AND_EFFECTS(OpName) \\ |
| if __package__ in {None, ""}: | ||
| here = Path(__file__).resolve() | ||
| for candidate in here.parents: | ||
| if (candidate / "ptodsl" / "__init__.py").exists(): |
There was a problem hiding this comment.
In latest main branch, you can install ptodsl package to the python env with pip install . --no-build-isolation. No need to manually search ptodsl path.
There was a problem hiding this comment.
Addressed. I removed the manual parent-directory sys.path probing from the example; it now assumes PTODSL is installed in the Python environment.
| if (candidate / "ptodsl" / "__init__.py").exists(): | ||
| sys.path.insert(0, str(candidate)) | ||
| break | ||
| else: |
There was a problem hiding this comment.
We already have some ST cases in test/tilelib-st. Maybe you can reuse them by just changing the target from a5 to a3.
| ) | ||
|
|
||
|
|
||
| def taddrelu(src0, src1, dst): |
There was a problem hiding this comment.
Is this a A2A3-only op? Should restrict its usage inside A2A3 kernels.
There was a problem hiding this comment.
Please also add documents for this new interface in the ptodsl/docs/user_guide directory.
There was a problem hiding this comment.
Addressed. I documented pto.tile.addrelu in ptodsl/docs/user_guide/08-compute-operations.md, including formula, supported dtypes, and the A2/A3-only restriction.
There was a problem hiding this comment.
Yes, this is A2/A3-only. I added a PTODSL tracing-time target guard, an IR verifier rejection for A5, and regression coverage for both PTODSL diagnostics and hand-authored PTO IR.
| @@ -0,0 +1,486 @@ | |||
| # Copyright (c) 2026 Huawei Technologies Co., Ltd. | |||
There was a problem hiding this comment.
The naming of this test suite is not clear. I suggest reuse the TileOp ST cases we already have in tilelib-st, which will be completed soon.
For vpto level test, you should place them under the test/vpto directory.
There was a problem hiding this comment.
I can move these tests under test/tilelib-st if that is the preferred repository layout, but I’m worried we would lose clarity and potentially coverage.
The current pytest suite is a comprehensive PTODSL e2e sweep for the new A2/A3 VPTO elementwise path. It covers binary, unary, scalar, bitwise, shift, fused, log, and reciprocal ops across multiple dtypes and dispatch shapes.
My concern with test/tilelib-st is that the folder name is not very descriptive for this purpose: these are specifically PTODSL end-to-end tests for the A2/A3 VPTO lowering pipeline. Moving them there may make that less obvious.
If you still prefer the ST layout, I can migrate them, but I’d like to preserve the current breadth of coverage.
| # ========================================================================= | ||
| # 5. Torch + torch-npu (for on-device kernel launch) | ||
| # ========================================================================= | ||
| RUN pip install --no-cache-dir torch==2.9.0 --index-url https://download.pytorch.org/whl/cpu \ |
There was a problem hiding this comment.
Better use torch==2.9.0.post2 or 2.10.0 here for CANN-9.0.0, see compatibility matrix here: https://gitcode.com/Ascend/pytorch/blob/master/README.zh.md#ascend-extension-for-pytorch%E7%89%88%E6%9C%AC%E9%85%8D%E5%A5%97%E8%A1%A8
There was a problem hiding this comment.
Good catch. The CANN 9.0.0 matrix pairs Ascend Extension for PyTorch 2.9.0.post2 with upstream PyTorch 2.9.0, so I kept torch==2.9.0 and updated torch-npu to 2.9.0.post2. I also updated the non-dev Dockerfile for consistency.
4 participants
A2/A3 VPTO Elementwise PR Summary
Summary
This branch adds the A2/A3 VPTO UB lowering path for tile elementwise operations and validates it end-to-end on A3 hardware. The implementation routes A2/A3 through the planned-memory UB pipeline (from A5 pipeline), lowers supported PTO tile ops to VPTO UB ops, and emits LLVM intrinsics.
What Changed
PTOViewToMemref -> PTOPlanMemory -> PTOResolveReservedBuffers -> PTOMaterializeTileHandles -> LowerPTOToUBufOps.tadd,tsub,tmul,tdiv,tmax,tmin,tand,tor,txor,tshls, andtshrs.tabs,trelu,tneg,texp,tlog,tsqrt,trsqrt, andtrecip.tadds,tmuls,tmaxs, andtmins.taddrelu, including PTO IR, UB IR, lowering, LLVM intrinsic emission, PTODSL wrappers, and tests.vaddrelu,vdup, andvln.alloc_tile addrvalues and models scratch usage for ops such astxor.pto.tile.*APIs.Validation
Verified after merging current
mainwith the LLVM 21 docker image:60/604/480/80192/192120/120Total A2/A3 shared VPTO elementwise hardware coverage validated on A3:
392tests.