x64: eliminate implicit narrowing conversions (full dim_t widening effort) - #5674
x64: eliminate implicit narrowing conversions (full dim_t widening effort)#5674tczeszun wants to merge 34 commits into
Conversation
github-actions
Bot
added
platform:cpu-x64
c6bc8f9 to
fbe4170
Compare
fbe4170 to
d6d578d
Compare
| double coffset = OCisR ? i2d(co[j]) : OCisC ? i2d(co[i]) : i2d(co[0]); | ||
| double val = ((*beta == 0.0f) ? 0.0 : f2d(*beta) * i2d(C[i + j * ldc])) | ||
| + f2d(*alpha) * dC[i + j * ldc] + coffset; | ||
| C[i + j * ldc] = q10n::out_round<int32_t>(q10n::saturate<int32_t>(val)); |
There was a problem hiding this comment.
(random spot)
I don't think this is the right approach for getting these changes promoted for the following reasons:
- There are currently 21 pull requests with cross-dependencies that require review. Changes made to one PR may require corresponding updates to others, which can invalidate review feedback that people have already spent time providing. Making these adjustments across 21 pull requests creates a lot of coordination overhead and makes the review process difficult to manage.
- Synchronizing changes from 21 pull requests into a single large PR introduces a high risk of rebase or integration mistakes that are difficult to identify in a 13k-line change. For example, if I request a change in a pull request I have no reliable way to verify that it was brought over correctly into the large PR. Attempting to do so would require reviewers to validate the same changes multiple times creating significant duplicate effort.
I reviewed the first pull request yesterday and today it has changed significantly. As a result, I had to review much of it again from scratch. I don't think this process is manageable.
I believe we should merge the 21 pull requests one by one. Keeping them marked as drafts during development is fine. When a pull request is ready it can be opened for review. Each pull request should be buildable and tested independently and given the relatively small scope of each change validation should not take much time.
However, I do not think all 21 pull requests should be open for review simultaneously. We should review, validate and merge each pull request before moving on to the next one. This provides a clear integration path, reduces the risks described above and ensures that each change is in good shape before additional dependent changes are introduced.
I believe this is a more practical and lower-risk approach for integrating a change of this size.
There was a problem hiding this comment.
Yeah, sorry for the force-push. After your comment #5565 (comment) it was needed to change almost every file again to make it in line with new conditions.
Initially the concern was about the number of changes, so splitting into per-primitive PRs was the most straightforward approach. Now, with cross-dependencies raised as a concern, I doubt it's possible to split further into PRs with no cross-dependencies that also pass the build independently. What if we end up with 2-3 PRs at 3-4k lines each? Wouldn't it just be easier to review in iterations - you review all the files, I fix them, you review again, and so on until there are no warnings/errors left? I'll try the per-dependency split, but going further than that feels like diminishing returns to me.
There was a problem hiding this comment.
Now, with cross-dependencies raised as a concern, I doubt it's possible to split further into PRs with no cross-dependencies that also pass the build independently. What if we end up with 2-3 PRs at 3-4k lines each?
To clarify, my concern is not the dependencies themselves. I don't think we need to split the changes any further. What I'm proposing is a different review process not a different pull request structure. The existing 21 pull requests can remain in draft state. We should review, test and merge one pull request first then open the next one for review. This keeps the review scope stable and avoids repeatedly revisiting earlier changes.
We can start with the 1st one that I've already reviewed: #5547
- jit_brgemm_primitive_conf: blocking fields to dim_t (nthr stays int) - brgemm_types: bcast_dim, load_dim, reduce_dim, LDA2/LDB2/LDC2, max_bs - brgemm_containers: insert/maybe_tile_configure params - brgemm.hpp: brgemm_kernel_execute bs param - brgemm_utils: nstl::min fixes, direct assignment for dim_t fields
Widen tensor-scale int fields in brgemm_desc_t (LDA/B/C/D, and the per-tensor block *counts* bdb/bdb2/ldb/ldb2/rdb) and brgemm_attr_t (hint_expected_*_size and related) to dim_t. Fixed-range fields that describe blocking/tiling structure rather than tensor extent (bd_block/ld_block/rd_block, the *_tail remainders, bd_block2/ld_block2, bs_group, vpad/bpad, bd_mask_level, hint_bd_block/hint_ld_block and friends) are documented and kept int. AMX tile-index helpers (get_bd_block2, get_ld_block2, get_num_*_tiles, get_*_tensor) stay int (bounded by AMX_TILES_NUM); only the wsp buffer size helpers (get_wsp_buffer_size and friends) and amx_buf_size_per_thread in jit_brgemm_primitive_conf.hpp/ jit_primitive_conf.hpp are widened to dim_t, since those scale with the tensor. Consolidate the one remaining narrowing at tc_configure_tile() (jit_generator.hpp): now takes dim_t and casts internally to the AMX hardware tile-register layout (uint8_t rows, uint16_t cols), instead of static_cast<int> at each call site. Fix cascading warnings in brgemm_utils.cpp, jit_brgemm_amx_uker.cpp, jit_brgemm_conv_utils.cpp, jit_brgemm_conv_bwd_utils.cpp, and jit_brgemm_inner_product_utils.cpp. Result: brgemm.cpp, brgemm_utils.cpp, and all matmul/inner_product/rnn brgemm-consumer files compile with zero -Wshorten-64-to-32 warnings. Conv-scope files and other AMX kernel callers verified to have zero new warnings from the cascade. Full library build passes with zero errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds template overloads of add/sub/cmp/imul constrained to dim_t, plus xbyak_register_index()/xbyak_address_scale() helpers, so brgemm JIT kernels can pass dim_t offsets/indices directly with narrowing and bounds checks consolidated at this single XByak boundary. shl intentionally does not get a dim_t overload: every shl call site only ever shifts by a small, hardware-bounded amount (a typesize or SIMD width), never a tensor-scale value, so those call sites cast explicitly instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Propagates brgemm/brdgmm/AMX-uker JIT kernel internals (block sizes, offsets, VMM/tile indices) from int to dim_t where they scale with a tensor dimension, using the new dim_t-safe XByak boundary overloads and xbyak_register_index()/xbyak_address_scale() for hardware-bounded register/scale values. Eliminates -Wshorten-64-to-32 warnings in these files. brgemm_containers.hpp/.cpp's kernel/palette container indices (insert()'s idx, operator[]'s idx, maybe_tile_configure()'s idx/new_idx) stay int: they are small bounded enumeration indices into a fixed-size container, not tensor-scale values, so they are kept/reverted to int here rather than widened. jit_brdgmm_kernel_base_t::simd_w_ and max_vmms_ stay int: they come from vlen/typesize and isa_num_vregs(), both hardware/ISA-bounded quantities, not tensor-scale values. tail_length() is narrowed back to int accordingly (its result is always < simd_w_, so the narrowing is provably safe). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix -Wshorten-64-to-32/-Wimplicit-int-conversion/-Wfloat-conversion/ -Wimplicit-float-conversion/-Wsign-compare narrowing warnings in the core JIT code-generation helpers shared by all brgemm kernels: - jit_generator.hpp: wrap Xbyak immediate/predicate arguments (vpinsrb, vpinsrd, vpextrd, vcmpps, shift amounts, etc.) passed through the uni_v* wrapper functions with explicit static_cast<uint8_t>/ Xbyak::uint8 at each call site. - jit_generator.cpp: cast the vpinsrw immediate argument to uint8_t. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group injector infrastructure, binary kernels, and required conversion fan-out. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group post-ops, transpose utils, and brdgmm buffer-size helpers. Widen JIT-codegen-time host loop trip counts and dimension-derived locals (nb, nb2, mb in post-ops; row_size_/tr_row_size_ chains in jit_brgemm_copy_to_coarse_t) from int to dim_t, keeping narrowing casts only at genuine hardware/register boundaries (masks, tails, block sizes, Xbyak register indices). Route the remaining simd_w_ narrowing through the dim_t-safe xbyak_register_index() helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group all matmul widening from this session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The qz_t specializations used a bare float 'beta' as a ternary condition (beta ? beta * out : 0), which triggers an implicit float-to-bool conversion warning. Use an explicit 'beta != 0.f' comparison instead; behavior is unchanged since testing a float for truthiness is equivalent to comparing it against zero. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group all inner-product widening from this session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Consolidate all RNN/GRU/LSTM narrowing-warning fixes (generic and x64 JIT) into a single commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group softmax widening from this session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group GEMM drivers, kernels, and x8s8s32x convolution helpers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…sions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
compute_vector_range takes int vmm indices (semantic range 0-31); start_idx/end_idx are dim_t from surrounding arithmetic and need an explicit boundary cast, same pattern used elsewhere for vmm indices.
d6d578d to
0abc01a
Compare
3 participants
Summary
Full
dim_tnarrowing-conversion cleanup across the x64 CPU JIT stack (BRGEMM infra, JIT kernel internals, injectors, BRGEMM, matmul, inner product, RNN, softmax, all conv variants, GEMM, reorder, xf16/fp8 conversion). This is the umbrella branch containing the full 27-commit linear history; the work has been split into stacked, individually reviewable PRs listed below (in dependency order).Note: this branch does not yet include @inteldimitrius narrowing-conversion fixes for the areas assigned to him (batch/group/layer normalization, LRN, pooling, PReLU, shuffle, resampling, reduction, binary) — those are being tracked and delivered separately, will be added to this branch soon. Added 2 temporary commits at the end, because build was failing because of dependencies.
Stacked PRs (in order)
Each PR is based on the previous one in this list, forming a fully linear chain that matches this branch's real commit history exactly (no synthetic/cherry-pick reconstruction) — merging them in order is equivalent to merging this branch.
Verification
cmake --build --target dnnl -j56, clang, Release): 0 errors, 0 warnings.-Wshorten-64-to-32 -Wimplicit-int-conversion -Wfloat-conversion -Wimplicit-float-conversion -Wsign-compare,-ferror-limit=0): 34 files show hits, but every one is either:int32_t↔float/float→boolquantization idioms inref_softmax.cppandref_deconvolution.cpp) — confirmed via diff that my commits there only touchedint/dim_tsignature narrowing and never touched these lines, no decision made yet on this.