Skip to content

RFC: Cleaning up macro naming, possibly switch to inline functions #293

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

Closed
pfalcon opened this issue Feb 15, 2014 · 16 comments
Closed

RFC: Cleaning up macro naming, possibly switch to inline functions #293

pfalcon opened this issue Feb 15, 2014 · 16 comments
Labels
enhancement Feature requests, new feature implementations

Comments

@pfalcon
Copy link
Contributor

pfalcon commented Feb 15, 2014

This is to capture #269 (comment) and following discussion.

Summary:

  1. Macros with THEIR CAPS break continuity in API. Suggested fix: downcase names of macros which are part of mp_obj_* API.
  2. As an extra step, consider leveraging C99 inline functions - both to replace macros and have inlinability guarantees for speed-critical accessors.
@dpgeorge
Copy link
Member

CAPS are good because they tell you it's a macro. I would rather make something a function than keep it a macro with lower case letters.

Are C99 inline functions mature/good enough to replace macros? (At least macros that don't use ## etc).

@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 15, 2014

CAPS are good because they tell you it's a macro.

But why anyone would bother to know it's a macro? The only reason, as mentioned in previous discussion, would be "caution, this thing may eval args with side-effects twice", but then macro should be written to be free from this double-eval (and most in uPy are likely already are).

Are C99 inline functions mature/good enough to replace macros?

With GCC, depending on optimization level, you may get inline functions not being inlined, which may be a feature or misfeature. To get 100% inlinability, one must use __attribute__((always_inline)), which then must be abstracted to clean looking define like INLINE or STRONG_INLINE or ALWAYS_INLINE. (And inline functions of course needs to be defined in headers.) So, there're warts, but otherwise, inlines are cool.

@dpgeorge
Copy link
Member

Okay, I'm happy to inline the MP_OBJ_IS_* functions.

For MP_OBJ_NEW_SMALL_INT, what if it's one day used to initialise static const data? Eg a static const list of uPy objects, some of which are strs, some ints?

@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 15, 2014

For MP_OBJ_NEW_SMALL_INT, what if it's one day used to initialise static const data?

Good question, worth trying, but as C++11 introduced "constexpr" modifier for that to work, I guess it's not going to work with inline functions. That means we should seriously consider just downcasing, and leave inlines for cool new usages, when such cool new usages arise ;-).

@dpgeorge
Copy link
Member

Okay, done some testing with macros vs inline functions. I considered the 3 "functions" MP_OBJ_IS_TYPE, MP_OBJ_IS_INT and MP_OBJ_IS_STR. I tried different ways of compiling them, and got the following binary size for stmhal:

  1. as a true function: 209808 bytes
  2. as a macro: 209824
  3. as a macros w/ single eval of all args: 209900
  4. as an inline static function: 210376
  5. as an inline static function w/ always_inline attribute: 211372 bytes.

So, it's almost 1500 bytes extra to inline, but only 16 bytes extra to make macros!

@pfalcon
Copy link
Contributor Author

pfalcon commented Apr 11, 2014

Pretty weird and something unright there. Dunno what. Switching to plain inlines also broke DEBUG=1 build. Using static inlines fixes that.

@dpgeorge
Copy link
Member

Yeah, I don't trust/understand the C compiler with it's use of inline.

The static inline I understand: see eg http://stackoverflow.com/questions/216510/extern-inline/216546#216546

@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 13, 2015

So, it's almost 1500 bytes extra to inline

I mentioned in some other comment that looking at some code for some port I got an impression that compiler may generate per-object instance of (non-inlined) inline function, i.e. the function body may be repeated multiple times in a final executable.

I just got reminded about it while reading "man gcov":

If the function is not inlined, the compiler must emit an out of line copy of the function, in any object file that needs it. If fileA.o and fileB.o both contain out of line bodies of a particular inlineable function, they will also both contain coverage counts for that function. When fileA.o and fileB.o are linked together, the linker will, on many systems, select one of those out of line bodies for all calls to that function, and remove or ignore the other.

So, what I saw is linker not removing other copies. Let this be anecdotal experience for now, but something to check on the next close look at the issue.

@stinos
Copy link
Contributor

stinos commented Mar 26, 2015

Is it possible this has to do with compiler/linker options? Maybe one needs to tell gcc optimize for size or so?

I just tried the same with msvc (replace MP_OBJ_IS_TYPE, MP_OBJ_IS_INT and MP_OBJ_IS_STR with normal inline functions), and the executable size stays exactly the same since the generated assembly is also exactly the same. Which is sort of logical, for such simple functions you'd expect the optimizer can see through the function calls and just replace it with bare instructions.

@vitiral
Copy link
Contributor

vitiral commented May 6, 2015

I use the inline declaration to make what are essentially macros public, without having to expose all macros, etc of the entire module.

I just ran into an issue where I tried to use inline function definitions and it wouldn't compile under the unix port. Are inline functions completely turned off? They definitely have uses!

If I change this line in both the header and C file:

mp_uint_t mem_first() ...

to

inline mp_uint_t mem_first() ...

The compiler fails with this error

CC ../py/gc.c
In file included from ../py/gc.c:53:0:
../py/mem.h:11:25: error: inline function ‘mem_first’ declared but never defined [-Werror]
 inline mp_uint_t        mem_first();
                         ^
../py/mem.h:11:25: error: inline function ‘mem_first’ declared but never defined [-Werror]
cc1: all warnings being treated as errors
make: *** [build/py/gc.o] Error 1

Are inline functions turned off?

@dpgeorge
Copy link
Member

dpgeorge commented May 6, 2015

@cloudformdesign you need to write static inline ....

@vitiral
Copy link
Contributor

vitiral commented May 6, 2015

no! what I needed was to declare it as inline in the .c file and use the extern keyword in the .h file. That didn't happen with my compiler.

See: http://www.greenend.org.uk/rjk/tech/inline.html

looking at this reference more, I think that both the .c and .h files are supposed to use extern inline. I'll have to experiment with this more.

@dpgeorge
Copy link
Member

dpgeorge commented Oct 9, 2015

I tried again to convert macros to inline functions. Using gcc 5.2.0. I can get a decrease of around 500 bytes on 64-bit unix port when using inline functions, and the same size (+/- 10 bytes) for stmhal. Everything except MP_OBJ_IS_TYPE is inlined, since inlining this adds around 500 bytes to stmhal.

Also, we can't inline MP_OBJ_NEW_SMALL_INT and MP_OBJ_NEW_QSTR because they are used to initialise constant data it many places.

So maybe we can downcase everything except those 2 above that must remain macros to initialise static data? And inline everything except MP_OBJ_IS_TYPE (which would become mp_obj_is_type)?

@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 11, 2015

Downcase - sure.

But to inline, relying on novel gcc version doesn't sound like a good idea. Nobody uses 5.2.0 currently and it will be few years till whole world starts to use it. So, if going back to inlining ideas, it would be good to understand what went right in the previous attempt, to be able to control the situation. (And I don't think it's a priority - there're lot of other things to do.)

@dpgeorge
Copy link
Member

But to inline, relying on novel gcc version doesn't sound like a good idea.

Yeah, I also don't like how inconsistent it is.

Anyway, main thing is that we have a plan to eventually work towards -- ie downcase and inline.

@dpgeorge
Copy link
Member

This is pretty much addressed by #4446, which converted most macros/inline functions to lowercase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests, new feature implementations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants