Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

Calling a function pointer generates two call instructions. #40

Closed
fvbommel opened this issue Jun 25, 2013 · 7 comments
Closed

Calling a function pointer generates two call instructions. #40

fvbommel opened this issue Jun 25, 2013 · 7 comments
Assignees

Comments

@fvbommel
Copy link

When you call a function pointer in Go, llgo currently generates two call instructions: one for the case that the function pointer is a closure, and one for the case it's not (selecting between them at runtime based on whether the context pointer is null). This causes unnecessary branching and code bloat.

I believe this can be avoided by marking the context parameter of closures with the nest attribute, and always treating calls to function pointers as calls to closures are currently treated, passing in the context pointer.
Because the nest parameter should effectively be ignored by functions that don't actually have a nest parameter (it's put into a register they don't consider a parameter register), this should work for regular functions that don't declare a nest attribute as well as for closures that do.
And even if this doesn't work because LLVM optimization passes don't like it, it would probably be easy enough to add an unused 'nest' parameter to each function type. It wouldn't cost anything at runtime as long as an 'undef' value is always passed to it when called directly.

@axw
Copy link
Member

axw commented Jun 26, 2013

Thanks for creating the issue. You're right, of course. I hadn't considered adding nest parameters (except back when llgo was using them to create trampolines), so thanks for the suggestions.

I considered doing something similar to what gc does, and just adding a (plain old) parameter to the front of each function; I haven't done this yet because I didn't want to add overhead to direct function calls, and doing this would make it more difficult to expose C functions to Go (i.e. I'm lazy).

@fvbommel
Copy link
Author

I spent a little time trying to implement this yesterday, but kept running into test failures. I guess I need to read the code some more, but I won't have time for it until at least this weekend.

@axw
Copy link
Member

axw commented Jun 26, 2013

Thanks for looking into it. I was just thinking about the nest approaches again.

I'm concerned about the use of nest on closures only. If the nest attribute is left of normal functions but presented to the caller, then it probably won't work with non-traditional targets like Emscripten (which isn't currently supported, but I'd like to be). I definitely do not want to break PNaCl support, but it should be fine AFAIK.

So I think I'd prefer if the additional parameter were added to all functions, perhaps with a way of marking functions as not having/needing one (i.e. "// #llgo somethingorother"). Is the nest attribute even useful then?

@fvbommel
Copy link
Author

If you want to put extra parameters on all functions there are two reasons I can think of to use the nest attribute:

  • Since nest parameters use an extra register there's no extra stack frame room reserved for it on targets that pass all parameters on the stack, and more chance all the other parameters to fit in registers on other targets. This is a (probably small) run-time cost avoided.
  • Calling C functions (as well as C functions calling Go ones) is less complicated, since using the nest attribute would keep the calling convention for "regular" functions the same as C assuming all the other stuff matches up. Note that the "other stuff" is quite complicated on some targets: look up the rules for passing structs on x86-64 sometime -- I once implemented those for LDC (the LLVM D Compiler).

I'd also like to note that IIRC the gc compiler actually does something extremely similar nowadays: it passes the context in a register that is never used to pass other parameters.

If you use the nest attribute there's no need to "mark" functions, AFAICT. (And avoiding special source-code annotations when they're not needed is a good thing, IMHO)
At runtime, you can just implicitly pass something to that nest parameter. If you're calling a function directly you know it's not used (since it can't be a closure) so you can pass an undef value at exactly zero run-time cost.
If you're calling via a function pointer, simply pass the context included in the pointer without checking if the function uses it, which is cheaper than the current strategy at the very least.

Note also that at least one LLVM optimization pass will actually replace arguments with undef values if it can prove they're not used, or just remove the parameter entirely from the function if it knows exactly where all the calls to it are. If we mark all unexported functions (except the ones from the runtime that are implicitly called all over the place) with one of the linkage attributes that means they can't be linked to from outside the package, that should work for every unexported function whose address is never taken.

Regarding your concerns about Emscripten: does it not support nest attributes at all, or does it simply not support calling function pointers through bitcasts that add nest parameters?
If it's the latter, putting the nest attribute on all functions would fix it, and since we're not using them to create trampolines it would be safe for Emscripten and other targets that don't support it to simply ignore the attribute.

IIRC PNaCl actually compiles stuff to machine code (eventually), so it should indeed probably be fine.

@axw
Copy link
Member

axw commented Jun 26, 2013

Thanks for the detailed reply. Sounds good to me.

My concern about Emscripten may be invalid, but I imagine it ignores the nest attribute given that it's not dealing with registers at all. That's only a problem if plain old functions aren't given the additional parameter. Not a difficult problem to solve.

@axw
Copy link
Member

axw commented Dec 23, 2013

This just became priority one, as it's holding up the ssa branch merge. Looking into it.

@axw
Copy link
Member

axw commented Dec 29, 2013

This is now done in the ssa branch, as at commit bea1627

I did not end up using nest, as I wanted to get it working sooner rather than later. This can be revisited later if there's a good reason to do so (e.g. as cited above, using a register that wouldn't otherwise be used).

@axw axw closed this as completed Jan 7, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants