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.
|
Retriggering CI for latest head c80521f. |
|
How many "non-elementwise" vector ops are left? (gather/scatter/reduce/...) Is it too big to also include them into this PR? |
deriveLoadCbufToCbControl / deriveLoadCbufToCaControl called the raw Type::getIntOrFloatBitWidth() accessor, which asserts on non-int/float element types (FP4 packed, hifloat8, f8, ...). In assert builds this crashed ptoas mid-pass (~60 VPTO/UB low-precision lit tests) with 'only integers and floats have a bitwidth'. Use pto::getPTOStorageElemBitWidth() instead, matching the sibling deriveLoadCbufToCaMxControl / deriveLoadCbufToCbMxControl functions and the rest of the codebase. The existing (elemBitWidth == 0 || % 8) guard already matches the helper's 0-on-unknown contract.
As you said, only the gather/scatter/reduce are missing. Not a large addition, I will add them in this PR. |
zhangstevenunity
left a comment
zhangstevenunity
left a comment
There was a problem hiding this comment.
Review: A2/A3 VPTO elementwise
Deep pass over the lowering (LowerPTOToUBufOps), the LLVM intrinsic emitter, the CLI/pipeline wiring, the ODS/verifiers/effects, and the PTODSL surface. Overall the A2/A3 elementwise path is well-built: the intrinsic emission is internally consistent with the design docs, the new ops (vaddrelu/vdup/vln) and the txor tmp-scratch effects modeling are correct, and most of the earlier Gemini review was against a stale commit and is addressed at HEAD.
The findings that stand out are all cases where this A2/A3 PR reaches into the shared A5 / VPTO device-emission path:
- P2
buildVPTOEmissionOptionsnow mis-targets A5 VPTO cube kernels (dav-c310-vecinstead ofdav-c310-cube). Inline onptoas.cpp:2651. - P2
usesCANN900Loweringflipped to unconditionalfalsedisables the CANN900 emitter for all targets/CANN versions, not just A3. Inline onVPTOLLVMEmitterDispatcher.cpp:15. - P2 The lowering silently drops each tile's valid-shape, so the valid-shape-aware dispatch modes (and the lit tests named after them) are effectively dead, and every op processes padding. Inline on
LowerPTOToUBufOps.cpp:252. - P3 The PTODSL native launch-wrapper compile doesn't receive
target_arch. Inline ontoolchain.py:173.
Verified clean (not bugs at HEAD)
txor'stmpis a required operand and WRITE-modeled ingetEffects; PlanMemory gives it a distinct buffer andLowerPTOToUBufOpsdecomposestxoratomically -> no aliasing/missing-sync bug (the recurring scratch-effect bug class is not present here).taddreluA2/A3-only:TAddReluOp::verify+dispatchVerifierByArchreject a5, and the dtype gatei16/f16/f32matches the availableVADDRELUintrinsics; PTODSL has a trace-time guard plus negative tests.vaddrelu/vdup/vlnintrinsic names, operand order, and config-word layouts match the design doc and the lit tests; the CBuf storage-bit-width fix (2ca1095) is sound.- Gemini's
== "dav-c220-vec"cube-exclusion flag is a false alarm: the elementwise op-pattern/legality sites (10994/11166) are vector-only by design andmarchis neverdav-c220-cubeon this path. Gemini's leftover-lambda compile error andVERITYtypo were on a stale commit and are not present at HEAD.
Lower-priority notes
- UB op verifiers only check the UB memory-role, not that
dst/srcshare element type/shape; a hand-written.ptowith mismatched pointer element types picks the intrinsic fromsrc0and miscompiles with no diagnostic (VPTOLLVMEmitter.cpp:4744). Compiler-generated IR is always consistent, so this is a robustness gap. set_maskhardware state is written byUBSetMask*but not read by the compute ops, so ordering is positional only; safe today (no scheduler on this IR post-lowering) but latent if one is added.vduprejects bf16 (clean compile error, not a miscompile); scalar-tile bf16 would pass an i64 bit-pattern toVMULS.bf16(out of scope per doc, unguarded)._ptoas_build_backend:build_editable(skip_install=True)no longer validates the runtime payload landed where the.pthpoints;_find_llvm_dirauto-probe may select an Ascend-shipped LLVM over the vpto-patched one on env-unset machines.
CI
The Codex review bot failed on INSUFFICIENT_BALANCE (not a code signal). No CI checks are reported on the branch, and the PR is currently CONFLICTING with main.
| options.targetTriple = "hiipu64-hisilicon-cce"; | ||
| options.cannVersion = cannVersion; | ||
| std::string arch = normalizeArch(targetArch); | ||
| options.march = isA2A3Arch(arch) ? "dav-c220-vec" : "dav-c310-vec"; |
There was a problem hiding this comment.
P2 (A5 VPTO regression, outside the PR's A2/A3 scope). buildVPTOEmissionOptions now unconditionally sets options.march, so for A5 it becomes the non-empty string "dav-c310-vec". On main this field was left empty.
Downstream, makeDeviceEmissionOptions (VPTOLLVMEmitter.cpp:11395) branches on options.march.empty(). With a non-empty march the Cube kernel kind now takes the else branch and gets aicoreArch = defaultTargetCPU = march = "dav-c310-vec" (vector); only defaultTargetFeatures resolves to the c310-cube string (line 11414-11415). ObjectEmission.cpp::resolveTargetCPU then reads that "dav-c310-vec" target-cpu attr and passes -mcpu=dav-c310-vec to bisheng for the cube object.
Net effect: ptoas --pto-arch=a5 --pto-backend=vpto <kernel with a cube section> emits the cube (AIC) device object targeting the vector (AIV) sub-core -- silent wrong-target codegen. On main the empty-march path correctly produced dav-c310-cube. A5 vector modules are unaffected. Not covered by the new tests (all a2/a3 vector).
Suggest only seeding march for a2/a3 and leaving it empty for a5 so the per-kind defaulting keeps working:
if (isA2A3Arch(arch))
options.march = "dav-c220-vec";
// a5: leave march empty; makeDeviceEmissionOptions picks vec/cube per kind|
|
||
| static bool usesCANN900Lowering(const CANNVersion &cannVersion) { | ||
| return cannVersion >= CANNVersion::release(9, 0, 0); | ||
| return false; // Force Beta1 path until CANN900 emitter is stable for A3 |
There was a problem hiding this comment.
P2 (behavior change beyond A2/A3 scope). This flips usesCANN900Lowering from cannVersion >= 9.0.0 to an unconditional return false, so every VPTO device emission -- including A5 on CANN 9.0.0+ -- is now routed through the Beta1 emitter instead of the CANN900 emitter, and lowerVPTOModuleToLLVMModulesCANN900 becomes dead code. The comment scopes the intent to A3, but the gate is global (no arch/version check).
If A5 (or other) VPTO targets on CANN9 previously went through the CANN900 path and the two emitters are not at parity, this is a silent codegen change for them, buried in an A2/A3 PR. Consider gating on isA2A3 (or on arch) rather than a blanket return false, or explicitly confirm A5 VPTO board/lit coverage still passes on CANN9 with Beta1.
| ctx, tbTy.getElementType(), | ||
| pto::AddressSpaceAttr::get(ctx, pto::AddressSpace::VEC)); | ||
| auto pc = builder.create<pto::CastPtrOp>(op.getLoc(), ptrTy, addr); | ||
| tileShapes[pc.getResult()] = SmallVector<int64_t, 2>(shape); |
There was a problem hiding this comment.
P2 (from code inspection -- the valid-shape dispatch is effectively dead). This rewrite stores only the padded shape into tileShapes, and the pto::PtrType branch of extractTileShapeInfoFromValue (line 156-161) leaves validShape empty. After this rewrite every compute-op operand is a PtrType, so extractTileShapeInfo computes vCols == cols and vRows == rows for all alloc_tile-derived tiles -- which is every lit test and the common kernel case.
Consequences:
-
In
dispatch(line 1437) thevCols == colsguard is then always true, so the entire non-continuous subtree --modeColVLAlign,modeCount2L,modeRowRpt,rowRptFast/rowRptChunked,headRows/tailRows, and thetailStride*helpers -- is unreachable. The lit tests named after those modes (tadd_col_vl_align,tadd_row_rpt[_tail],tadd_chunked[_tail], andtadd_count) actually lower throughmodeNorm1L/modeCount1L/modeSmall; they pass only because the CHECKs are structural (no test pins the emittedset_maskcount for av_col < colsshape). So the valid-shape paths have no real coverage. -
Every op is lowered against the padded width, so it reads/writes the padding columns (uninitialized UB) rather than just
[0, v_col). Valid-region results stay correct only because tiles are allocated at padded size (PlanMemory usesgetShape()), but it processes up tocols/v_colx more data and touches uninitialized memory (e.g.vln/vrsqrt/vdivover padding).
Separately, the unary/scalar/dup paths (dispatchUnary/dispatchShift/dispatchDup, ~line 933-1079) iterate flat over vRows*vCols with no per-row cols stride, unlike the binary dispatch. That is only safe because validShape is currently dropped (flat == full contiguous buffer). If a valid-shape-preserving TileBufType tile (e.g. a non-alloc block-arg) with v_col < cols and v_row > 1 ever reaches a unary/scalar op, those paths would skip rows / process padding as valid (wrong result), because they lack the guard the binary path has.
Suggestion: capture the tile's valid shape into tileShapes (or key the dispatch off the alloc_tile valid_row/valid_col), and add a lit test that pins the set_mask count for a padded multi-row shape so these modes get real coverage.
|
|
||
|
|
||
| def aicore_arch_for_kernel_kind(kernel_kind: str) -> str: | ||
| def aicore_arch_for_kernel_kind(kernel_kind: str, target_arch: str = "a5") -> str: |
There was a problem hiding this comment.
P3 (cross-layer completeness). This adds the target_arch parameter (and test_runtime_toolchain.py calls it directly), but the only production caller -- _kernel_compile_flags(kernel_kind) in native_build.py:126 -- calls aicore_arch_for_kernel_kind(kernel_kind) without target_arch, so it always defaults to "a5" and emits --cce-aicore-arch=dav-c310-vec for the launch.cpp wrapper. build_native_library threads module_spec.target_arch into the kernel-object compile (_run_ptoas) but not into _compile_launch_cpp.
So on A2/A3 the kernel object is built for dav-c220 while the launch stub is built for dav-c310. The new unit test passes but never exercises the real call path, giving false confidence. (The A3 e2e apparently still links/runs, so the wrapper's arch may not be load-bearing, but the two objects should agree.) Thread module_spec.target_arch through _compile_launch_cpp -> _kernel_compile_flags -> aicore_arch_for_kernel_kind.
zhangstevenunity
left a comment
zhangstevenunity
left a comment
There was a problem hiding this comment.
I found five correctness issues while reviewing the A2/A3 VPTO lowering and PTODSL integration. Inline details and reproductions are included below.
| ctx, tbTy.getElementType(), | ||
| pto::AddressSpaceAttr::get(ctx, pto::AddressSpace::VEC)); | ||
| auto pc = builder.create<pto::CastPtrOp>(op.getLoc(), ptrTy, addr); | ||
| tileShapes[pc.getResult()] = SmallVector<int64_t, 2>(shape); |
There was a problem hiding this comment.
Only the physical shape is stored before replaceAllUsesWith converts every tile operand to pto.ptr. From then on extractTileShapeInfoFromValue takes the pointer path, has no validShape, and falls back to vRows=rows/vCols=cols. I reproduced this with the existing 4x96 f32, v_col=32 test: lowering emits six 64-element iterations (all 384 physical elements) instead of operating on 4x32 valid elements with a 96-element row stride. Please preserve both physical and valid shapes in the pointer metadata and make the test assert the iteration count and offsets.
| int64_t totalRpts = (totalV + epr - 1) / epr; | ||
| b.create<pto::UBSetMaskCountOp>(loc); | ||
| b.create<pto::UBSetMaskOp>(loc, i64c(totalV, loc, b), i64c0(loc, b)); | ||
| emitUBBinOp<UBop>(loc, b, dst, s0, s1, i64c(totalRpts, loc, b), i64c8(loc, b)); |
There was a problem hiding this comment.
dispatch calls this helper only when totalRpts > 255, yet the helper passes that value straight to the UB op. The 16x1024 f32 test lowers to repeat = 256; LowerUBufBinary masks repeat with 0xff, so it is encoded as zero. The new design document also says C220 count mode does not support repeat > 1. Please split this into repeat-1 chunks or a loop and update the current CHECK-NOT: scf.for expectation.
| b.setInsertionPointAfterValue(view); | ||
| DmaViewInfo info; | ||
| auto ptrTy = pto::PtrType::get(ctx, memTy.getElementType(), msAttr); | ||
| Value root = traceRootMemRef(view); |
There was a problem hiding this comment.
At this point PTOViewToMemref has already converted partition_view to memref.subview. Tracing to the root here discards the subview offset, while the following code rebuilds row-major strides and fills all offsets with zero, also discarding non-contiguous layouts. With a 17x32 view partitioned at [1, 0], lowering computes a zero byte offset instead of 128 bytes. Please extract strided metadata and accumulate the real view offsets and strides rather than reconstructing them.
| } | ||
|
|
||
| void fullMask(Location loc, OpBuilder &b) { | ||
| b.create<pto::UBSetMaskOp>(loc, i64cM1(loc, b), i64c0(loc, b)); |
There was a problem hiding this comment.
fullMask emits mask0=-1, mask1=0, but this PR's own UBSetMask documentation says full-write mode is -1, -1; mask1 controls lanes 64-127. Consequently, an f16 operation following a masked or count-mode operation can leave the upper half disabled. Please restore both mask words and add a chained masked-to-normal regression test.
|
|
||
|
|
||
| def aicore_arch_for_kernel_kind(kernel_kind: str) -> str: | ||
| def aicore_arch_for_kernel_kind(kernel_kind: str, target_arch: str = "a5") -> str: |
There was a problem hiding this comment.
The new target_arch argument is never passed by native build: _kernel_compile_flags still calls aicore_arch_for_kernel_kind(kernel_kind), and _compile_launch_cpp does not receive module_spec.target_arch. A2/A3 launcher code is therefore still compiled for dav-c310-* while PTOAS emits a C220 kernel. Please thread target_arch through the call chain and test the actual Bisheng command rather than only this helper.
# Conflicts: # tools/ptoas/ptoas.cpp
6 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.