conv2d v300验证 - #75
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68f4ccd0ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const int Mb = gM / tM; | ||
| const int Nb = gN / tN; | ||
| const int Kb = gK / tK; |
There was a problem hiding this comment.
When any global dimension is not divisible by its tile dimension, these floor divisions silently omit the remainder; the newly added default build uses gM = 14 * 14 = 196 with tM = 16, so four spatial positions are never computed for every output channel. The identical calculation in conv2d_rm.hpp has the same limitation, so both kernels should implement tail tiles or enforce divisibility at compile time.
Useful? React with 👍 / 👎.
| nz_mask = result_2d != 0 | ||
| nz_count = int(nz_mask.sum().item()) | ||
| nz_diff = diff[nz_mask] | ||
| nz_max = float(nz_diff.max().item()) if nz_count > 0 else 0.0 | ||
| nz_mean = float(nz_diff.mean().item()) if nz_count > 0 else 0.0 | ||
| mismatch = int((nz_diff > atol).sum().item()) if nz_count > 0 else 0 | ||
| passed = (mismatch == 0 and nz_count > 0) |
There was a problem hiding this comment.
When a kernel leaves output elements unwritten or incorrectly writes zero, those elements are removed by nz_mask, and passed becomes true as long as any remaining nonzero element matches. This specifically masks the partial-store and tail failures that the diagnostic branches below already recognize, causing the verifier to return a successful process status for incomplete convolution results.
Useful? React with 👍 / 👎.
| npu = np.fromfile(args.npu, dtype=np.float32) | ||
| golden = generate_golden(args.gM, args.gN, args.K) | ||
| compare(npu, golden, args.label or f"K={args.K}") |
There was a problem hiding this comment.
When compare() detects a mismatch and returns False, main() discards that value and reaches the end normally, so this command always exits with status 0. Any automation invoking this comparator therefore treats a printed FAIL as a successful verification; return a nonzero status when the comparison fails.
Useful? React with 👍 / 👎.
| verdict = 'PASS' if mismatch == 0 and int(nz.sum()) == res.size and not has_inf and not has_nan else 'FAIL' | ||
| print(f' Verdict: {verdict}') |
There was a problem hiding this comment.
When the result contains mismatches, missing values, infinities, or NaNs, the embedded Python only prints Verdict: FAIL and still exits with status 0. Consequently set -e does not stop run_test.sh, and experiment runners see every completed verification as successful regardless of the computed verdict.
Useful? React with 👍 / 👎.
| # Print compile command | ||
| print(f"\nCompile command:") | ||
| print(f" make TYPE={args.dtype.upper().replace('FP', 'CONV_')} \\") |
There was a problem hiding this comment.
For both supported inputs this substitution prints TYPE=CONV_32 or TYPE=CONV_16, while the added Makefile only defines sources and targets for TYPE=FP32 and TYPE=FP16. Following the generator's displayed command therefore builds no conv2d ELF; print the original uppercased dtype instead.
Useful? React with 👍 / 👎.
| parser.add_argument('--kernel-h', type=int, default=3, help='Kernel height') | ||
| parser.add_argument('--kernel-w', type=int, default=3, help='Kernel width') |
There was a problem hiding this comment.
Running this generator with its defaults creates 3x3 weights and a 12x12 reference output, but the associated kernel and Makefile implement only a 1x1 convolution and ignore the printed KH, KW, stride, padding, and dilation arguments. The resulting binaries cannot validate that kernel and will have incompatible weight and output sizes; either default and restrict generation to 1x1 or implement the advertised convolution parameters.
Useful? React with 👍 / 👎.
| tilM=$tM tilN=$tN tilK=$tK TESTCASE=conv2d_rm PLAT=linx \ | ||
| CC_OPTS="-DRES_CHECK -DENABLE_BINARY_OUTPUT -include /tmp/conv2d_rm_chk_dir.h" 2>&1 | grep -v "^Makefile:" | tail -1 | ||
|
|
||
| echo "--- Running ${LABEL} ---" |
There was a problem hiding this comment.
When compilation fails, the pipeline's status comes from tail rather than make because the script enables set -e but not pipefail. If an ELF from a previous invocation exists, the script can consequently continue and verify that stale binary as though it were the newly requested configuration; enable pipefail or capture and check the make status before filtering its output.
Useful? React with 👍 / 👎.
| tM=$6; tN=$7; tK=$8; DATA_DIR=$9; LABEL=${10} | ||
|
|
||
| export COMPILER_DIR=/mnt/workspace/v310/linx-toolchain-build/output/linx_blockisa_llvm_musl/bin |
There was a problem hiding this comment.
In every checkout not located at /mnt/workspace/v310/SuperNPUBench, this overrides the caller's compiler configuration and points make, comparison files, and the ELF lookup at a different or nonexistent tree. The repository-wide compilation entry point already accepts COMPILER_DIR and derives its root from the script location, so this test should likewise use caller-provided tool paths and compute ROOT relative to itself.
Useful? React with 👍 / 👎.
| input_fp32 = (torch.randn((1, IN_C, IN_H, IN_W), generator=gen) * 0.1).clamp(-1, 1) | ||
| weight_fp32 = (torch.randn((OUT_C, IN_C, 1, 1), generator=gen) * 0.1).clamp(-1, 1) | ||
|
|
||
| output = F.conv2d(input_fp32, weight_fp32) |
There was a problem hiding this comment.
For TYPE=FP16, the files supplied to the kernel are rounded to float16 later, but this golden convolution is computed first from the original FP32 tensors. Accuracy comparisons therefore include operand-quantization differences as apparent kernel errors, especially as IN_C and the accumulation depth grow; quantize the operands to the selected dtype before computing the reference.
Useful? React with 👍 / 👎.
| // The weight data in src1.bin must be pre-converted: each tK x tN tile block | ||
| // of B is transposed, so that NORM TLOAD (no B.DATR) + ColMajor ZN physical | ||
| // layout produce the correct tile register contents via double-transpose. |
There was a problem hiding this comment.
These instructions require callers to pre-convert each weight tile to BFractal layout, while the accompanying conv2d_rm_gen.py explicitly says no pre-conversion is needed and writes a standard row-major W^T. A caller following this kernel documentation will therefore feed a different layout from the shipped verifier and obtain permuted multiplication results; make the documented contract agree with the tested data generator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c49b7eb64
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| verdict = 'PASS' if mismatch == 0 and int(nz.sum()) == res.size and not has_inf and not has_nan else 'FAIL' | ||
| print(f' Verdict: {verdict}') |
There was a problem hiding this comment.
When the multi-PE output contains mismatches, zeros, infinities, or NaNs, this embedded Python only prints Verdict: FAIL and then exits successfully, so run_test.sh still returns status 0 and automation records a failed experiment as passing. The identical verifier in conv2d_rm_mt/src/run_test.sh has the same behavior; explicitly exit nonzero when verdict is FAIL.
Useful? React with 👍 / 👎.
| res = np.fromfile('$TMPDIR/dump.bin', dtype=np.float32) | ||
| if res.size == 0: | ||
| print(' ERROR: dump.bin is empty!') | ||
| exit(1) |
There was a problem hiding this comment.
When gfrun produces a nonempty but truncated dump, this check accepts it and the later verdict can report PASS whenever every available prefix value equals the expected constant, because it compares coverage only with res.size rather than the required gM * gN. Require the exact expected element count before comparing; the batch verifier in conv2d_rm_mt/src/run_test.sh likewise needs 4 * gM * gN elements.
Useful? React with 👍 / 👎.
| # Compile all operators | ||
| compile_operator "$REPO_ROOT/test/kernel/matmul" "matmul" | ||
| compile_operator "$REPO_ROOT/test/kernel/broadcast" "broadcast" | ||
| compile_operator "$REPO_ROOT/test/kernel/conv2d" "conv2d" |
There was a problem hiding this comment.
Running the repository-wide compilation entry point builds only test/kernel/conv2d; the newly added conv2d_rm, conv2d_rm_dyn, conv2d_rm_mt, and conv2d_rm_shared directories are never invoked anywhere in compile_all.sh. Consequently the advertised full build neither produces those ELFs nor detects regressions in most of the kernels added by this commit, even though the latter two already provide their own compile.all scripts.
Useful? React with 👍 / 👎.
| volatile uint32_t* vi = (volatile uint32_t*)input_nchw; | ||
| for (int i = 0; i < kPeNum * IN_C * IN_H * IN_W; ++i) | ||
| vi[i] = 0x40000000u; |
There was a problem hiding this comment.
Because every element of all four PE input matrices is initialized to the same value, the batch-of-four test cannot detect incorrect tid offsets, reuse of PE0's input, or permutation of the four outputs: all of those failures still produce the same scalar expected by run_test.sh. Initialize each PE with distinguishable data and compare each output separately so this test actually validates the new batch-level mapping.
Useful? React with 👍 / 👎.
| conv2d_1x1_tileop<datatype, IN_C, IN_H, IN_W, OUT_C, | ||
| tilM, tilN, tilK>(output, input_nchw, weight); |
There was a problem hiding this comment.
This executable includes benchmark.h but invokes the kernel without BENCHSTART/BENCHEND, so Linx performance tooling receives no TRACE.begin/TRACE.end region for the convolution and cannot report a kernel-scoped benchmark measurement. The new conv2d_rm.cpp and conv2d_rm_dyn.cpp drivers omit the markers in the same way, while the accompanying multi-PE drivers correctly bracket their calls.
Useful? React with 👍 / 👎.
| header_path = SCRIPT_DIR / "conv2d_embed_data.h" | ||
| generate_embed_header(header_path, input_bin, weight_bin) |
There was a problem hiding this comment.
When two FP16 verification cases run concurrently, both overwrite the same tracked conv2d_embed_data.h immediately before invoking separate builds, so one build can embed the other case's input and weight arrays while retaining its own compile-time dimensions. That race can produce incorrect comparisons or out-of-bounds kernel reads for differently sized cases; generate the header under the case directory and pass a case-specific include path to the compiler.
Useful? React with 👍 / 👎.
1 participant
conv2d: add one-level conv2d and conv2d_rm kernels with tests