Skip to content

Softmax update#1494

Open
bugracyln wants to merge 51 commits into
fastmachinelearning:mainfrom
bugracyln:softmax_updated
Open

Softmax update#1494
bugracyln wants to merge 51 commits into
fastmachinelearning:mainfrom
bugracyln:softmax_updated

Conversation

@bugracyln

@bugracyln bugracyln commented Jun 25, 2026

Copy link
Copy Markdown

Description

📝 Please include a summary of the change.

The softmax table generation logic was updated. The implementation for writing the softmax tables was revised, and memory attributes were added to enable a more efficient FPGA compilation flow. In addition, the templates were modified to use weights directly from the configuration.

  • Please also include relevant motivation and context.

The primary motivation for these changes was to bring the oneAPI backend closer to the Vivado backend in terms of implementation.

Memory attributes were added to enable memory banking on the FPGA, allowing for more efficient memory access. The weights are now copied directly into the configuration so that the compiler can recognise the entire table as a set of fixed values. This enables the memory to be implemented more efficiently, resulting in improved resource utilisation during FPGA compilation.

  • List any dependencies that are required for this change.

N/A

Type of change

For a new feature or function, please create an issue first to discuss it
with us before submitting a pull request.

Note: Please delete options that are not relevant.

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation update
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)
  • A new research paper code implementation
  • Other (Specify)

Tests

📝 Please describe the tests that you ran to verify your changes.

The changes were primarily verified using black-box tests on an isolated softmax unit. Testing was performed for both quantised and non-quantised implementations. For the quantised version, both configurations, with and without exp and inv table quantisers (QuantiserConfig(...)), were tested.

Additional testing included:

  • Generating FPGA RTL reports.
  • Building the emulator.
  • Performing a hardware compilation using the new Intel oneAPI compiler.

This PR currently supports only the Intel oneAPI compiler. Support for the Altera HLS compiler will be added in a future PR.

The implementation was also evaluated with different table sizes, and the resulting RTL reports were inspected to verify improvements in resource utilisation.

  • Provide instructions so we can reproduce.

A Python test file and a Keras model containing only a single softmax layer (Softmax or QSoftmax) were used. For the quantised implementation, the input and output quantisers for the exp and inv lookup tables were configured using QuantiserConfig(...). Tests were run with both the quantisers enabled and disabled.

The test configuration included:

  • Standard Softmax and QSoftmax models.
  • Explicit exp and inv table input output quantisation.
  • FPGA RTL generation.
  • Emulator build.
  • Hardware compilation with the Altera HLS (newer version of Intel oneAPI)compiler.
  • Please also list any relevant details for your test configuration.

Test Configuration:

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the oneAPI backend softmax implementation to generate per-layer lookup tables as compile-time constants and wire them into the generated configuration, aiming to align the oneAPI flow more closely with the Vivado backend and improve FPGA compilation/resource utilization.

Changes:

  • Generate per-softmax-layer exp/inv lookup tables as headers and auto-include them into parameters.h.
  • Update oneAPI softmax C++ templates to use lookup tables from CONFIG_T instead of #included .tb fragments.
  • Extend oneAPI softmax config generation with per-table sizing/type plumbing and add a multidimensional softmax helper.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
hls4ml/writer/oneapi_writer.py Generates and includes per-layer softmax exp/inv table headers during oneAPI project emission.
hls4ml/templates/oneapi/firmware/parameters.h Adds an insertion point for writer-generated softmax table includes.
hls4ml/templates/oneapi/firmware/nnet_utils/nnet_activation.h Reworks stable softmax to use CONFIG_T tables and adds a multidim helper implementation.
hls4ml/templates/oneapi/firmware/nnet_utils/nnet_activation_stream.h Reworks streaming stable softmax to use CONFIG_T tables and cleans up type aliases.
hls4ml/backends/oneapi/passes/core_templates.py Extends softmax config generation with exp/inv table wiring and sizing/typing logic.
hls4ml/backends/oneapi/oneapi_backend.py Removes the prior oneAPI softmax multidimensional io_parallel restriction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hls4ml/writer/oneapi_writer.py Outdated
Comment on lines +730 to +741
ac_type = layer.get_attr('inp_norm_t')

if ac_type is not None:
try:
fp_bits = ac_type.precision.integer + ac_type.precision.fractional
fp_integer = ac_type.precision.integer
fp_signed = ac_type.precision.signed
except Exception:
# FixedPrecisionType wasn't correctly stored in layer attributes, use default values
pass
if fp_signed is False:
raise Exception('Softmax types need to be signed')

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this logic since it’s no longer needed

Comment thread hls4ml/writer/oneapi_writer.py Outdated
Comment on lines +792 to +803
ac_type = layer.get_attr('inv_inp_t')

if ac_type is not None:
try:
fp_bits = ac_type.precision.integer + ac_type.precision.fractional
fp_integer = ac_type.precision.integer
fp_signed = ac_type.precision.signed
except Exception:
# FixedPrecisionType wasn't correctly stored in layer attributes, use default values
pass
if fp_signed is False:
raise Exception('Softmax types need to be signed')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will all go away with #1476, so either incorporate things from there or ignore it for now.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged with #1476 so this logic is not needed

Comment thread hls4ml/backends/oneapi/passes/core_templates.py
Comment on lines +269 to +276
// *************************************************
// Multidimensional Softmax
// *************************************************

// Helper to remap the config for the core softmax function
template <class CONFIG_T> struct softmax_multidim_slice_config : CONFIG_T {
static constexpr unsigned n_in = CONFIG_T::n_slice;
};

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These added now as multidim backend is functional.

Comment thread hls4ml/backends/oneapi/oneapi_backend.py
if params['type'] == 'softmax':
# The lookup input (x - x_max) is always <= 0, so only the negative half
if 'exp_table_size' in params and params['exp_table_size'] is not None:
params['exp_table_size'] //= 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not divide this in half. The table size as given already takes the unsignedness into account. It doesn't make sense to expect people to give you twice the size of what they want implemented.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this problem by assuming positive only types for softmax tables

params['exp_table_size'] //= 2
else:
# Use the default precision
params['exp_table_size'] = 2 ** (params['table_t'].precision.width - 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default parameters should be defined https://github.com/fastmachinelearning/hls4ml/blob/main/hls4ml/backends/fpga/fpga_backend.py#L130 and similar. Note that the defaults are updated in #1476. I would remove all these updates here. You set the defaults in the attribute, not when reading the attributes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this logic, respecting the new defaults

params.setdefault('table_size', params['exp_table_size']) # Not sure if necessary

# Determine accumulator type if present, else derive it yourself based on the input size.
if params['accum_t'].name == 'model_default_t':

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in infer_precision.py. I think the change is in #1476 already so probably not needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Comment thread hls4ml/writer/oneapi_writer.py Outdated
# the signed fixed-point input range is ever addressed.
# Therefore only half of the full address space is required.
table_size = (
int(layer.get_attr('exp_table_size')) // 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again the //2 should be removed. The exp_table_size already takes that into account. It doesn't make sense to pass twice that value. Also, at this point it should be required to be defined. It should not be None.

@bugracyln bugracyln Jul 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed division by 2

Comment thread hls4ml/writer/oneapi_writer.py Outdated
except Exception:
# FixedPrecisionType wasn't correctly stored in layer attributes, use default values
pass
if fp_signed is False:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table type being signed is something that will go away. The defaults are all unsigned. You can either leave as is for now, or try to handle the usual, unsigned kind. See #1476 and the Vivado implementation.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed these checks entirely

Comment thread hls4ml/writer/oneapi_writer.py Outdated
table_size = self.__get_table_size(model, 'softmax')
ac_type = layer.get_attr('inp_norm_t')

if ac_type is not None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense. fp_bits is just ac_type.precision.width. It can't be None. If it's old leftover code, that's fine, but it will go away once #1476 incorporates oneAPI.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this logic

Comment thread hls4ml/writer/oneapi_writer.py Outdated
Comment on lines +792 to +803
ac_type = layer.get_attr('inv_inp_t')

if ac_type is not None:
try:
fp_bits = ac_type.precision.integer + ac_type.precision.fractional
fp_integer = ac_type.precision.integer
fp_signed = ac_type.precision.signed
except Exception:
# FixedPrecisionType wasn't correctly stored in layer attributes, use default values
pass
if fp_signed is False:
raise Exception('Softmax types need to be signed')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will all go away with #1476, so either incorporate things from there or ignore it for now.

Comment thread hls4ml/writer/oneapi_writer.py Outdated
copyfile(srcpath, dstpath)

def __get_table_size(self, model, activation):
def __get_table_size(self, model, activation, table_name='table_size'):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this comment in the other PR. What does a table name being called table_size mean? They seem to be different things. The naming needs to be updated. It would be equally funny to have table name be table_dimension, for example.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now changed table_name to size_attr_name instead, since that is the dictionary key that we use to obtain the table size.

@jmitrevs

jmitrevs commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This now includes #1476 since they both work on softmax. This way hopefully conflicts can be reduced.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 2, 2026
params['type'] = node.get_attr('activation')

if (params['type'] == 'softmax') or (params['type'] == 'softmax_multidim'):
params.setdefault('n_inner', 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These options have a default value, so there's no reason to set it here. See the default value in fpga_backend.py

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed n_inner and n_outer


if 'exp_table_size' not in params:
params['exp_table_size'] = 2 ** params['inp_norm_t'].precision.width
node.set_attr('exp_table_size', params['exp_table_size'])

@jmitrevs jmitrevs Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not make the default different for oneAPI vs Vitis/Vivado. Note that the setting there is:
params.setdefault('exp_table_size', params['table_size']). I am worried that this is too big of a default if, for example, we have a width of 18 bits.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could argue for a different default, but let's not diverge.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to use the default size of 1024 bits, but I would suggest something around 4096, perhaps, since at 1e-3 tolerance, I get around 60% of the values wrong. This is with a non-quantised layer too, since we don't have defined exp_table and inv_table sizes for a non-quantised layer, we default to 1024, leading to very coarse table values.


else:
# TODO: For latency check the table sizes correctly, match them
if 'exp_table_size' not in params:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is re-implementing set_default.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got rid of it :)

template <class data_pipe, class res_pipe, typename CONFIG_T> void softmax_legacy_stream() {
#include "activation_tables/exp_table_legacy.tb"
#include "activation_tables/invert_table_legacy.tb"
//#include "activation_tables/exp_table_legacy.tb"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest removing the commented out includes that are just historic.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 3, 2026
def __write_exp_table(self, model, path):
table_name = 'exp_table'
table_size = self.__get_table_size(model, 'softmax')
def __get_table_precision(self, model, activation, table_name='table_precision'):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function used?

fp_bits = 16
fp_integer = 6
fp_signed = True
def __write_exp_table(self, model, path):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, feel free to ignore, but would it be useful for this to have a _stable suffix. Also, would the name be better if it was made plural, write_exp_tables_stable (and similar for the other ones) since this now can write out multiple tables.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 7, 2026
real_val = f.exp_float()
h_file.write(sep + str(real_val))
sep = ', '
# Default fixed point precision

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for default here.

h_file.close()
# Exp table should use the same precision as exp_table, as seen in Vivado code
# init_exp_table<data_T, CONFIG_T>(exp_table);
for layer in model.get_layers():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why loop again for layers here within the loop for layers on line 824? This looks like a bug.


h_file.write('};\n')
h_file.close()
# Exp table should use the same precision as exp_table, as seen in Vivado code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the comment, that the exp table should use the same precision as exp table. Isn't that true inherently?

sep = ', '
# Default fixed point precision, in case values from layer attributes cannot be extracted
# 8 bits for integer part, 10 bits for decimal - total, 18
fp_bits = 18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I don't think you need defaults here.

h_file.close()
# Invert table should use the same precision as exp_table, as seen in Vivado code
# init_invert_table<typename CONFIG_T::exp_table_t, CONFIG_T>(invert_table);
for layer in model.get_layers():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And again a layer loop inside of a layer loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please test Trigger testing by creating local PR branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants