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

Pseudo Computed Goto #272

Closed
wants to merge 3 commits into from
Closed

Conversation

BarabasGitHub
Copy link
Contributor

Since it is not possible to use computed gotos in Visual Studio/VC++ I have simulated the dispatch table with a switch case containing goto statements.

@munificent
Copy link
Member

Interesting!

How does this affect the performance? Can you run the benchmarks with and without this change and show us what the numbers look like?

@BarabasGitHub
Copy link
Contributor Author

There are some numbers in the discussion on issue #269
If you want I can try to run the benchmarks with the old and new approach later this week.

@BarabasGitHub
Copy link
Contributor Author

Sorry I'm a bit late, but some things came up. In any case I ran the benchmark script, with the original to generate a base-line and then with the new implementation. (This is with visual studio.)

Original:

binary_trees - wren            .......... 0.49s 0.0092 no baseline
delta_blue - wren              .......... 0.23s 0.0036 no baseline
fib - wren                     .......... 0.62s 0.0027 no baseline
for - wren                     .......... 0.31s 0.0017 no baseline
method_call - wren             .......... 0.41s 0.0100 no baseline
map_numeric - wren             .......... 0.80s 0.0133 no baseline
map_string - wren              .......... 0.20s 0.0024 no baseline
string_equals - wren           .......... 0.68s 0.0045 no baseline

New:

binary_trees - wren            .......... 0.42s 0.0048 118.27% relative to baseline
delta_blue - wren              .......... 0.19s 0.0013 121.51% relative to baseline
fib - wren                     .......... 0.55s 0.0033 113.09% relative to baseline
for - wren                     .......... 0.26s 0.0030 122.18% relative to baseline
method_call - wren             .......... 0.29s 0.0018 138.31% relative to baseline
map_numeric - wren             .......... 0.69s 0.0042 115.32% relative to baseline
map_string - wren              .......... 0.18s 0.0016 107.73% relative to baseline
string_equals - wren           .......... 0.54s 0.0027 125.09% relative to baseline

@munificent
Copy link
Member

OK, I'm sold. :)

I wish we didn't have to turn wren_opcodes.h into a giant macro, but I can't think of any other solution.

I'll add some other comments at specific points, but would you mind also rebasing this patch against the latest master? I think some stuff has moved under you, making this hard to merge in.

Thank you for doing so much work on this!

// No computed gotos in Visual Studio.
#define WREN_COMPUTED_GOTO 0
// No real computed gotos in Visual Studio. Use the pseudo version.
#define WREN_COMPUTED_GOTO 2
Copy link
Member

Choose a reason for hiding this comment

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

Instead of numeric values, let's do separate defines, I'm thinking:

WREN_DISPATCH_SWITCH
WREN_DISPATCH_COMPUTED_GOTO
WREN_DISPATCH_JUMP_TABLES

SWITCH is the 0 case here, COMPUTED_GOTO is 1, and JUMP_TABLES is 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that will indeed be more clear.

@BarabasGitHub
Copy link
Contributor Author

I also really dislike the wren_opcodes macro. I wish it could be different.

Anyway, I tried to rebase the goto branch onto the new master branch. But somewhere along the line I did something that made my master branch end up being the same as the goto branch. I think I might have pushed it to the wrong branch or so... AND I fixed it!

I didn't change your suggestions. Should I do that?

@MrSapps
Copy link

MrSapps commented May 11, 2016

Did you try what a function pointer table to declspec(naked) functions looks like with MSVC? Since you can get the address of the function thats like taking the address of a label, then the "naked" function call should become a jmp?

@BarabasGitHub
Copy link
Contributor Author

I don't think that will work. It says on msdn: "The naked attribute
affects only the nature of the compiler's code generation for the
function's prolog and epilog sequences. It does not affect the code that is
generated for calling such functions. Thus, the naked attribute is not
considered part of the function's type, and function pointers cannot have
the naked attribute." (
https://msdn.microsoft.com/en-us/library/21d5kd3a.aspx)

On 11 May 2016 at 09:18, Paul notifications@github.com wrote:

Did you try what a function pointer table to declspec(naked) functions
looks like with MSVC? Since you can get the address of the function thats
like taking the address of a label, then the "naked" function call should
become a jmp?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#272 (comment)

@MrSapps
Copy link

MrSapps commented Jun 17, 2016

I saw something else related to this today "__assume" https://msdn.microsoft.com/en-us/library/1b3fsfxw.aspx apparently you could use this on the switch value to stop it generating the extract checks. Might be worth checking if you ever get time.

Edit: Nevermind you are already using this

@munificent
Copy link
Member

Ugh, I'm so sorry, but I left this open so long that I'm not able to merge it in now since so many changes have happened since. :(

@munificent munificent closed this Mar 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants