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

core: Add IS_CT_CONSTANT() #17273

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 25, 2021

Contribution description

This adds a simple macro to check (at C level) whether a given expression is proven to be compile time constant and suitable for constant folding. This allows writing code like this:

int gpio_read(gpio_t pin) {
    if (IS_CT_CONSTANT(pin)) {
        /* this implementation should even be able to use the port and
         * pin number as immediate in inline assembly */
    }
    else {
        /* less efficient implementation that cannot use port and pin
         * number as immediate in inline assembly */
    }
}

Testing procedure

A unit test was provided that should do the trick.

Issues/PRs references

None

@maribu maribu added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Nov 25, 2021
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Nov 25, 2021
@maribu
Copy link
Member Author

maribu commented Nov 25, 2021

I added a WIP because I haven't tested this yet for the motivating example stated (for using pin / port as immediate value in AVR assembly when the GPIO is a compile time constant).

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 13, 2021
@benpicco
Copy link
Contributor

still WIP?

@maribu maribu removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 14, 2021
@maribu
Copy link
Member Author

maribu commented Dec 14, 2021

No longer, I found a new user for it :)

{
/* These test might fail on non-GCC-non-clang compilers. We don't support
* any of those (yet), but this test might need adaption in the future */
int actual_constant = (int)(42 * 1337 / 3.14159);
Copy link
Contributor

@benpicco benpicco Dec 14, 2021

Choose a reason for hiding this comment

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

Suggested change
int actual_constant = (int)(42 * 1337 / 3.14159);
unsigned actual_constant = (unsigned)(42 * 1337 / 3.14159);

to make MSP430 happy

Copy link
Member Author

Choose a reason for hiding this comment

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

That was not enough to make GCC 11.2 for the AVR happy, I changed the computation a bit to not overflow. In addition, I moved this into at static inline function to make it a bit less trivial, so that we have a decent base-line on what we expect from compilers to detect as suitable for constant-folding.

@maribu maribu force-pushed the core/IS_CT_CONSTANT branch 2 times, most recently from 7bb2f27 to 5e4785c Compare December 14, 2021 17:27
@maribu
Copy link
Member Author

maribu commented Dec 14, 2021

$ make BOARD=arduino-mega2560 tests-kernel_defines flash test -C tests/unittests
make: Entering directory '/home/maribu/Repos/software/RIOT/tests/unittests'
Building application "tests_unittests" for "arduino-mega2560" with MCU "atmega2560".

[...]

Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
(): This is RIOT! (Version: 2022.01-devel-723-g5e4785-core/IS_CT_CONSTANT)
Help: Press s to start test, r to print it is ready


main(): This is RIOT! (Version: 2022.01-devel-723-g5e4785-core/IS_CT_CONSTANT)
Help: Press s to start test, r to print it is ready
READY
s
START
....
OK (4 tests)

make: Leaving directory '/home/maribu/Repos/software/RIOT/tests/unittests'

🎉

This adds a simple macro to check (at C level) whether a given
expression is proven to be compile time constant and suitable for
constant folding. This allows writing code like this:

```C
int gpio_read(gpio_t pin) {
    if (IS_CT_CONSTANT(pin)) {
        /* this implementation should even be able to use the port and
         * pin number as immediate in inline assembly */
    }
    else {
        /* less efficient implementation that cannot use port and pin
         * number as immediate in inline assembly */
    }
}
```
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 7, 2022
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK

@aabadie aabadie added Process: needs >1 ACK Integration Process: This PR requires more than one ACK and removed Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Jan 7, 2022
@aabadie aabadie enabled auto-merge January 7, 2022 14:47
@aabadie aabadie merged commit 5083061 into RIOT-OS:master Jan 7, 2022
@maribu maribu deleted the core/IS_CT_CONSTANT branch January 13, 2022 11:51
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants