requantSymInt32TensorToScale can emit qMin - 1 under SR (no integer clamp after rounding)

Open Beginner friendly
  • Is something wrong?
#511 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
62/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
c
Domain
documentation, machine-learning

Research direction

The fault is in requantSymInt32TensorToScale in src/tensor/TensorConversion.c, which clamps in float and then rounds with no integer clamp. Compare it with the integer clamps in convertFloatTensorToSymInt32Tensor and requantSymInt32Tensor in the same file. The doc sentence to update is in docs/conventions/arithmetic-sym.md under "Fixed-scale requant". Done when the fixed-scale path applies an integer clamp after rounding, the doc matches, and the existing tests still pass. The rounding tie in #510 is a separate change.

Written by the indexing model from the issue text.

Description

bug

Summary

Under SR_HALF_AWAY, requantSymInt32TensorToScale (the fixed-scale requant) can emit a code one below the SYM_INT32 grid minimum. A value saturated to qMin can round to qMin - 1, e.g. -2049 at int12.

Where

In src/tensor/TensorConversion.c, requantSymInt32TensorToScale clamps in the float domain and then rounds, with no integer clamp afterwards:

outputInt32[i] =
    roundByMode(clamp(((float)inputInt32[i] * inScale) / targetScale, qMin, qMax),
                outputSymInt32QConfig->roundingMode, rs);

The other SYM_INT32 and BFP emit sites round and then apply clampInt32(..., qMin, qMax). Examples are convertFloatTensorToSymInt32Tensor, requantSymInt32Tensor, and the BFP value-domain packs.

Repro

Take a value far below the grid, e.g. -5000 in int12 units. The float clamp maps it to -2048. Stochastic rounding then evaluates round(-2048 + u - 0.5), and for u == 0 this is round(-2048.5) = -2049.

The probability is about 2^-24 per saturated negative element. The root cause is the SR tie behaviour in #510 (roundSRHalfAway with u == 0).

Suggested fix

Add an integer clampInt32(..., (int32_t)qMin, (int32_t)qMax) after roundByMode, as every other emit site does. The behaviour under HALF_AWAY is unchanged. The fix is still worth making even if the SR rounding itself is changed.

The behaviour is documented as-is in docs/conventions/arithmetic-sym.md ("Fixed-scale requant") on the F9 stack. Update that sentence together with the fix.

Found by the differential numerics harness work (the F9 stack). It is pre-existing.

Dominant language
C
Stars
1
Forks
3
Avg merge
8h 46m
Merged PRs (30d)
19

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from es-ude/OnDeviceTraining

All issues in es-ude/OnDeviceTraining

Similar issues

More C issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.