Repository navigation
Conversation
A Dense layer with a fused Keras softmax activation kept activation='softmax', so its (never written) tables were included in parameters.h and compilation failed.
The sum of exponentials is cast to inv_inp_t before the inverse table lookup, as in the stable implementation, so a user can tune the lookup range for a latency softmax. The default inv_inp_t remains a copy of accum_t, so default results are unchanged. The inv_inp_t integer-bit override now applies only to stable, where the sum is bounded by the number of inputs.
|
Detailed review of the current state of the softmax changes by Claude: Review at 68aa4e6Thanks for folding #1552 in, and for 68aa4e6. With the latency inverse table addressed by Three things I'd like fixed before merging:
With the suggested changes for (2) and (3) applied, those stable cases drop to 7 and 21 flips (main: 822 and 31), all the softmax HGQ cases below are bit-exact, and the Vitis subset of 1. Heterogeneous quantizers in io_streamhls4ml/hls4ml/backends/fpga/passes/hgq_proxy_model.py Lines 74 to 83 in 68aa4e6 This used to raise The collapse also loses information. The width is I'd keep the error, and move any io_stream relaxation to its own PR since it touches every FPGA backend. Suggested change for L74–83: if model.config.config['IOType'] != 'io_parallel':
raise NotImplementedError('Heterogenous quantization for activations is only supported with IOType=io_parallel')2. Model-level configs skip type inferenceLines 1058 to 1065 in 68aa4e6 With Treating types that only resolve to the model default as # A standalone Keras Softmax layer arrives as 'Softmax'
self.set_attr('activation', self.get_attr('activation').lower())
# Internal types that only resolve to the model-wide default are inferred instead
if not self.get_attr('_bit_exact', False):
for name in ('exp_table', 'inv_table', 'inv_inp', 'inp_norm', 'accum'):
current = self.get_attr(f'{name}_t')
if current is not None and current.name != 'model_default_t':
continue
if self.model.config.get_precision(self, name)[1] == 'model_default_t':
self.set_attr(f'{name}_t', NamedType(f'{self.name}_{name}_t', UnspecifiedPrecisionType()))
# set the needed types if needed3. TableSize cap on bit-exact tableshls4ml/hls4ml/backends/fpga/passes/fix_softmax_table_size.py Lines 25 to 28 in 68aa4e6 This caps bit-exact (HGQ) stable tables at If the aim is to bound table memory, I'd keep bit-exact tables full size and warn, so the user decides. My #1531 sizes them to exactly 2^width, so settling this here unblocks that PR too. The change below makes the case above bit-exact again and keeps the tests passing. It needs if node.get_attr('_bit_exact', False) and max(exp_table_size, inv_table_size) > table_size:
# Bit-exact (HGQ) tables address every value of their input type; shrinking them breaks bit-exactness
warnings.warn(
f'Softmax layer {node.name}: bit-exact tables ({exp_table_size}, {inv_table_size} entries) '
f'exceed TableSize={table_size}; keeping them to stay bit-exact.',
stacklevel=1,
)Smaller items
Lines 209 to 212 in 68aa4e6 This now only lands when the user sets 1-bit hls4ml/hls4ml/backends/fpga/passes/hgq_proxy_model.py Lines 16 to 33 in 68aa4e6 (The same widening is in
Leftover debug code hls4ml/hls4ml/converters/keras_v3/hgq2/softmax.py Lines 124 to 129 in 68aa4e6 A Shared template mutated in place hls4ml/hls4ml/backends/altera/passes/core_templates.py Lines 277 to 280 in 68aa4e6 This assigns to the shared template instance. After the first stable softmax, every later softmax in the process (any implementation, any model) is formatted with the stable template. That's harmless today because the stable template is a superset, but it's easy to avoid. Suggested change for L277–280: if params['implementation'] == 'stable':
return (softmax_config_template + softmax_config_table_template_stable).format(**params)
return self.template.format(**params)Nits
Not in the diff
What I ranAt 68aa4e6, on Linux with g++ csim and no vendor tools:
|
…softmax code The stable inv_inp_t override changed the integer bits of the configured type in place. With only a model-level precision that type is the shared model_default_t, so the generated code kept the original typedef while the Altera writer built its tables from the adjusted precision. The override now creates a layer-specific type, with an extra bit if signed. Also remove the unreachable inp_norm_t fallback and unused accum_t_str in the Vivado softmax config template, the unused table_t typedef in the Altera softmax config, and a leftover pdb comment in the HGQ softmax handler.
_set_type_t resolved types through get_precision, which falls back to model_default_t, so with a model-level config the softmax internal types and the activation table_t took the model precision instead of the attribute's declared default. The model-wide default no longer overrides a declared default; the softmax defaults (now including accum) are auto, so infer_precision handles them as for name-level configs.
Capping bit-exact (HGQ) stable softmax tables at TableSize dropped low address bits and broke bit-exactness. They are now kept at full size, with a warning when they exceed TableSize. Add test_hgq2_softmax.py to check this on Vitis and Altera.
The inv_inp_t integer-bit override in _set_type_t rewrote explicitly configured types and used max(shape) rather than n_slice. It is replaced by a bound in _infer_softmax_precision, so it applies only when inv_inp_t is inferred: the stable sum of exponentials is at most n_slice. Also remove the now unreachable model_default_t substitution in the Vivado softmax config template.
ac_fixed has no 1-bit type. Instead of widening 1-bit quantizers to a signed 2-bit type, which changes rounding and saturation, keep homogeneous 1-bit FixedPointQuantizers on Altera io_parallel and give the mask function an explicit 1-bit path that casts through ac_fixed<2,I+1,k> and selects the exact 1-bit value. The remaining type-level widening in fpga_types keeps the signedness and warns that the 1-bit range is no longer enforced.
The softmax_multidim kernels were never called: multidimensional io_stream softmax runs through softmax_stream, and io_parallel rejects it. Drop them, the n_slice/n_outer/n_inner config fields only they read, the unreachable softmax_multidim check and the commented-out selection in the HGQ converter.
The io_stream kernels on Altera and Vitis normalize each stream element, i.e. the last dimension, so softmax along any other axis compiled but gave wrong results on Altera and failed an assert at runtime on Vitis. On Vitis io_parallel, an axis that does not match n_outer and n_inner also gave wrong results. Raise at conversion time instead, and canonicalize the positive form of the last axis to -1 so it passes the Vitis stream assert.
A stable softmax assigned its template to the shared template instance, so every later softmax in the process was formatted with the stable template. Format the stable template without storing it. Also fix the implemenation typo in fix_softmax_table_size.
|
I tried to address the review comments. Let's see how the pytests go. I am ignoring Quartus failures since that backend will go away. |
| axis = layer.get_attr('axis', -1) | ||
| # 'axis' counts the batch dimension, the input shape does not | ||
| last_axis = axis in (-1, len(shape)) | ||
| n_slice = int(np.prod(shape)) // layer.get_attr('n_outer', 1) // layer.get_attr('n_inner', 1) |
There was a problem hiding this comment.
n_outer and n_inner are only set by the keras frontends, so we should add that to the torch and onnx ones, otherwise multidimensional softmax with io_stream will be rejected for those frontends.
There was a problem hiding this comment.
Should be fixed now.
Latency softmax: why the inferred types lose to main at small logitsA correction to my earlier review first: I wrote that latency with inferred types is less accurate than main's fixed defaults. That only holds for small logits. Across logit scales it's a trade-off, and the loss traces to one thing: the inverse table isn't resized along with the inferred Mechanism
Swapping only Measured Mean absolute error vs float softmax, latency,
So this branch is worse than main only at σ ≤ 1, and 5–40× better at σ ≥ 2. Two possible fixes
My preference is A with a cap, falling back to B's saturating type above the cap. Happy to go with B if table memory is the bigger concern. With wide inputs such as |
Only the Keras frontends set n_outer and n_inner, so multidimensional softmax from PyTorch and ONNX was normalized over the whole tensor with io_parallel and rejected with io_stream. Set them from the softmax axis, and move the axis and recompute them in the channels_last conversion. Drop the PyTorch io_stream axis check, which ran before the conversion; the backends now check the axis in the final layout.
The latency inverse table is indexed by the top bits of inv_inp_t, which was inferred as a copy of accum_t. Each integer bit added for range halved the lookup resolution, so small logits lost accuracy. Cap the inferred integer bits at what the table covers with a step of 0.25 and saturate larger sums. Explicitly set inv_inp types are left alone, and a larger InvTableSize widens the inferred type.
|
For the Latency softmax, I modified what is inferred by auto for the precision--explicit precision are not modified. So I chose option 2 for when auto inference is used. A user can explicitly grow the table size and set the precision if they desire. This seems a cleaner solution for the default. |
|
I also encountered a significant accuracy drop after softmax while working through the Xilinx/Vitis flow with a pretrained MNIST LeNet-1 model. Using the Vitis backend with I tested this PR ( This summary were prepared with assistance from Codex (GPT-6.1 Sol). |
|
Hi, thanks for hls4ml! Some numbers that might help with this PR. Setup: hls4ml 1.3.0, Vitis, io_parallel, default softmax_stable, 4-bit QKeras MLP (15→32→32→5), jet-tagging dataset, 10 seeds. The cause seems to be inv_inp_t = fixed<18,8>: the 1/x table is indexed by its top 10 bits, which leaves 0.25-wide bins over a sum in [1, 5]. Note: in 1.3.0 setting this via the config is ignored. Editing the node before compile() works: from hls4ml.model.types import RoundingMode, SaturationMode
p = hls_model.graph[softmax_name].get_attr('inv_inp_t').precision
p.width, p.integer, p.signed = 18, 3, False
p.rounding_mode, p.saturation_mode = RoundingMode.RND, SaturationMode.SAT
hls_model.compile()Details: https://github.com/Datayx/qnn-fpga. Happy to rerun on the #1542 branch. |
Description
This combines #1476 and #1494. Also, due to many merge conflicts, it uses the renamed Altera backend of #1536, so it should only be merged after.
The
legacysoftmax was prone to overflow, so this PR made it use an accumulator type. Also, forstableandlatency, the default types are better chosen.For Altera, table generation logic was updated to be per-layer, not global, and copied from the configuration Memory attributes were added to enable memory banking on the FPGA, allowing for more efficient memory access.
Type of change
Tests
This updates
test_softmax.pybecause previously it set attributes that had no effect.We need to add Altera to the CI. Otherwise, other tests should not fail. However, Quartus tests were not a focus since that beckend will be retired with #1537. (It may introduce merge conflicts, but those should be fairly easy to solve.)
Checklist
pre-commiton the files I edited or added.