littlekernel / lk

LK embedded kernel
MIT License
3.12k stars 613 forks source link

What's up with "-finline" in engine.mk ? #250

Closed zinahe closed 4 years ago

zinahe commented 4 years ago

Hi,

This could be due to lack of knowledge; so be gentle. I have not been able to find an -finline option in GCC. One that comes closest to it is -finline-functions.

Grateful if someone could shed some light on this.

mu578 commented 4 years ago

Hello,

performs c++ inlining, however that's up to the compiler (according to the context) to respect it or not.

travisg commented 4 years ago

Yah, it goes pretty far back, and it may be unnecessary with modern gccs, but the main idea is to try to force the compiler to honor inline, even if being built with -O0.

It may be mostly unnecessary now that inline is part of both C and C++, but in the older days you really needed to switch it on or it might not do it.

On Tue, Jul 23, 2019 at 1:17 PM Juggling Frog notifications@github.com wrote:

Hello,

perform c++ inlining.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/littlekernel/lk/issues/250?email_source=notifications&email_token=AAAX3LFCNR6HWCRBVHIPR3TQA5ROZA5CNFSM4IGFPIAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2UKAMA#issuecomment-514367536, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAX3LD2OQFH4WVP4WMDBU3QA5ROZANCNFSM4IGFPIAA .

zinahe commented 4 years ago

Thank you for the prompt reply. Why not remove it then ? It's not like you'd be breaking any backwards compatibility.

Food for thought.

nvll commented 4 years ago

If it's not hurting anything it's fine to leave it there rather than risking unexpected regressions by removing it.

mu578 commented 4 years ago

@hello, I think my comment has been misinterpreted;

1- you must keep it; else when it's legit it won't be honored. 2- let's write a binomial coefficient recursive function for integers

inline unsigned int nchoosek(unsigned int n, unsigned int k)
{
    if (n < UINT_MAX && k < UINT_MAX) {
        if (k > n) {
            return UINT_MAX;
        } else if ((k == 0) || (k == n)) {
            return 1;
        } else if ((k == 1) || (k == n - 1)) {
            return n;
        }
        return (n * nchoosek(n - 1, k - 1)) / k;
    }
    return UINT_MAX;
}

conclusion: in this case, we all do understand why the inlining request won't be honored.

You have a good day.

advice: that's always good to list and check the symbol table of one build.

travisg commented 4 years ago

Really, I'd just need to be convinced it's not needed on modern toolchains (say 4.x gcc series) for both C and C++. If so, then it'd be worth dropping. Otherwise it's probably harmless.

On Wed, Jul 24, 2019 at 11:14 AM Juggling Frog notifications@github.com wrote:

@hello https://github.com/hello, I think my comment has been misinterpreted;

1- you must keep it; else when it's legit it won't be honored. 2- let's write a binomial coefficient recursive function for integers

inline unsigned int nchoosek(unsigned int n, unsigned int k) { if (n < UINT_MAX && k < UINT_MAX) { if (k > n) { return UINT_MAX; } else if ((k == 0) || (k == n)) { return 1; } else if ((k == 1) || (k == n - 1)) { return n; } return (n * nchoosek(n - 1, k - 1)) / k; } return UINT_MAX; }

conclusion: in this case, we all do understand why the inlining request won't be honored.

You have a good day.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/littlekernel/lk/issues/250?email_source=notifications&email_token=AAAX3LADCXCKVCCMZ7ZBZUDQBCLWZA5CNFSM4IGFPIAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2XFLOY#issuecomment-514741691, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAX3LAGGIWQ56HIY27EIB3QBCLWZANCNFSM4IGFPIAA .

mu578 commented 4 years ago

@hello use -Winline and you'll know.

zinahe commented 4 years ago

Again; apologies if I didn't make my question clear. I was not asking if -finline-functions is useful or not in general. I was interested to know why it is written as -finline in engine.mk. I had no idea whether it a short-hand for -finline-functions or it means something completely different.

@moe123, Anyway, in an effort to answer my own question, I compiled lk with -Winline. I got the exact same binary with -finline, with -finline-functions and with both removed. GCC reported the same number of warnings failing to inline some functions (148 to be exact).

I'm not sure what it all means. But if one gets the same result with or without -finline then what is it doing ?

[Edit] Setting ARCH_OPTFLAGS to O2 and O3 made no difference [/Edit]

mu578 commented 4 years ago

@hello, it depends on your optimization flags see documentation for property inheritance; + don't mixup c and c++ optimization flags; @see Os; however, if you want to enforce inlining policy in certain cases __attributes__ are your friends, inline keyword these days means: you compiler; do whatever you want, I just casted a wish; but sometimes it does not add up like you would.

Note there is a difference between __inline and inline keywords; see documentation.

zinahe commented 4 years ago

Setting ARCH_OPTFLAGS to -O2 and -O3 made no difference.

swetland commented 4 years ago

I believe the situation that lead to the use of -finline originally was that under -O0 (and possibly -Os?) the compiler would not honor the inline keyword (as Travis mentions upthread), leading to compilation errors at those optimization levels.

doug65536 commented 4 years ago

If you want to force inline in -O0, use inline __attribute__((__always_inline__))

travisg commented 4 years ago

I finally removed the -finline switch. A bit late to get to it, but it was indeed extraneous.

Looks like it literally does nothing except to cancel an existing -fno-inline switch on the command line. GCC internally tracks it literally as flag_no_inline and defaults to 0.