esl-epfl / x-heep

eXtendable Heterogeneous Energy-Efficient Platform based on RISC-V
Other
146 stars 81 forks source link

Replace `inline` with `static inline` #582

Closed cousteaulecommandant closed 2 months ago

cousteaulecommandant commented 2 months ago

Some header files were defining functions as inline rather than static inline, which causes the compile/link process to fail at certain optimization levels (-O0). In some cases, an extern definition is added to a .c file which fixes the issue, but this is not an ideal solution, and in many cases new inline functions have been added to the header file and not to the .c file, causing compilation with -O0 to fail.

This commit solves that issue by replacing all inline declarations with static inline, and removing the extern declaration if it existed (sometimes removing an entire C file which only contained these definitions). Note that all the functions declared this way were small functions, so this change won't cause a noticeable increase in the program size.

This fixes issue #481.

davideschiavone commented 2 months ago

@cousteaulecommandant , thx for your contribution, I am asking @JoseCalero to check it.

please also check that the C++ support is not broken,

best Davide

cousteaulecommandant commented 2 months ago

My understanding is that this should work fine in C++, since the behavior of static inline is essentially the same as in C.

(In C++ you can also use inline without static and not have the issue described here, since the function definition will be available to the linker. But adding static in C++ won't hurt.)

JoseCalero commented 2 months ago

@cousteaulecommandant using static inline is fine for me, i checked the PR and it is OK. The only thing I need to check a little bit more in detail is that functionality-wise by saying that these functions are static inline, everytime they are called from any other unit (.c file), a new copy of that function is created... I guess that for the specific functions you changed, it does not affect functionality, but we have to be careful with that so not to change all the inlines to static inline. Could you also check the correct conflicts? As soon as this is done, I think we can merge

cousteaulecommandant commented 2 months ago

why did you change this? can you change it back?

Since I removed that file, I thought it made sense to remove it from that IDE project file. Maybe I shouldn't have. Anyway, reverted. :)

everytime they are called from any other unit (.c file), a new copy of that function is created

That'd be the case anyway since they're being inlined. If they're not being inlined (-O0), then that's an error (which this PR fixes), except for the cases where the inline functions were being declared as extern in a .c file. In those cases, with no inlining, my understanding is that a copy of the function will indeed be created, and called; this could be fixed with __attribute__ ((always_inline)) which will force the inlining, but I don't think that's necessary (that would result in a different behavior with -O0 since it would skip the function call, resulting in fewer clock cycles, but functionally the result would be the same).

I guess that for the specific functions you changed, it does not affect functionality

Unless I'm missing something, the functional and timing behavior will be the same. The executable size would be different, though:

Could you also check the correct conflicts?

Done. A recent commit replaced a few timer-related inline functions with regular ones, so this fix no longer applied to those functions.

Does it make sense to make those timer functions non-inlined though? That'll potentially interfere with the time they measure (more than inlining them).