Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove global flags under BASOP_NOGLOB #173

Draft
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

sdoehla-iis
Copy link

ITU-T’s basic operators inside the STL (G.191) are the basis of speech coding standards, offering a complete set of operators of relevance for embedded fixed-point implementations. A deficit however is the use of global variables that leads to diverging implementations to satisfy current best practices in software development for thread-safe and portable implementations. This contribution addresses this aspect, triggered by 3GPP’s needs for a set of operators without this deficit and a recent pull request for STL in the Open ITU Github project. This is also described in ITU-T contribution SG12-C-0170.

This pull request implements the removal of global flags, similarly to #171 , but based on the code that has been developed as part of the IVAS development work (see https://forge.3gpp.org/rep/ivas-codec-pc/ivas-codec for details on IVAS).

A prior version of this been integrated into the IVAS floating-point code (now standardized as 3GPP TS 26.258, available here: https://www.3gpp.org/dynareport/26258.htm ). This version has been a rewrite that is fully compatible to the code already in IVAS, but covering all operators that are available in STL2023. This version is an in-place modification (differing from #171 that adds extra files), which could break software that relies on those operators if they would now use the here proposed version. This is for the reason that:

  • overflow or carry in the existing operators now triggers an assert()
  • new operators are added that are permitted to set/get the Overflow/Carry flags
  • the Overflow/Carry flags now being a function parameter instead of being global.

Software that does not trigger the Overflow or Carry flags would not be affected, other software would need some adaptations, as an example, the add operator would be complemented with a new variant allowing overflow to be signalled explicitly via a supplied variable. Here the name is amended with a suffix “_o” for “overflow”.

`Word16 add_o (Word16 var1, Word16 var2, Flag * Overflow);`

The original operator Word16 add (Word16 var1, Word16 var2); is complemented with an assert, such that execution halts in debugging variants when an overflow is triggered.

The benefit is that developers would be made fully aware of when an overflow occurs, instead of going undetected as in the existing STL BASOP. If an overflow is foreseen, the variant which allows overflows shall be used instead. The behavior of the original operator would otherwise remain identical in release builds, just without setting of the Overflow flag or Carry flag.

it should be noted that G.722 as part of STL was also adapted. It uses the BASOPs and also triggered Overflows at four places in the code. Further verification on a large data set would be desirable.

The pull request also tackles the exit() calls to some extent, but it is at this stage undecided what a reasonable approach to tackle those is. In case of undefined behaviour (division by zero) currently abort() is used. This may still be changed further to e.g. call exit(136) to indicate SIGFPE (which despite the name is also raised for divisions) or some other indication that there is undefined behaviour and the application is thus in an undefined state that normally requires abort.

@signalogic
Copy link

signalogic commented Sep 25, 2023

Signalogic has published a BASOP upgrade proposal. As a non-member we can't upload the doc to the ITU site so we have posted it on our STL Github fork:

https://github.com/signalogic/STL/blob/dev/doc/Signalogic_ITU_Non-Member_SG12_BASOPS_Upgrade.pdf

@ludomal
Copy link
Member

ludomal commented Feb 26, 2024

@signalogic @sdoehla-iis @ErikNorvell-Ericsson, would you be able to give us an update on how things are going on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants