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

Optimize NdParseOperand #102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

turol
Copy link
Contributor

@turol turol commented Aug 31, 2024

NdParseOperand is taking up a lot of cycles. This seems to be because of the unpredictable switches. This is an attempt to make it faster by handling "regular" cases with arrays. It makes the branches more predictable but only has a slight effect on performance.

    N           Min           Max        Median           Avg        Stddev
x  30      16655663      18165255      18069974      18030108     265051.76
+  30      17821354      18267074      18140863      18136979     93104.058
Difference at 95.0% confidence
	106871 +/- 102683
	0.592735% +/- 0.569509%
	(Student's t, pooled s = 198646)
Instructions/second, higher is better

It might be possible to speed it up more by re-numbering ND_OPERAND_SIZE_SPEC and ND_OPERAND_TYPE_SPEC so the regular cases are either first or last.

@turol
Copy link
Contributor Author

turol commented Aug 31, 2024

Looks like MSVC doesn't like the empty initializers. I don't have MSVC to test with, what kind of syntax does it want?

@ianichitei
Copy link
Contributor

Looks like MSVC doesn't like the empty initializers. I don't have MSVC to test with, what kind of syntax does it want?

It should work with { 0 } instead of { }.

That table looks like it might be auto-generated by isagenerator, will have to see what @vlutas thinks about this.

@vlutas
Copy link
Collaborator

vlutas commented Sep 2, 2024

There are several problems with this change:

  1. it makes the code very hard to follow; operands processing should be as homogenous as possible
  2. each time a new operand size or type is added, we'd have to make sure to update the array(s) as well
  3. the internal operand type & size enums are exactly that - internal - and they can change any time; changing them would require the arrays to also be modified (although this could be mitigated via designated initializers)
  4. it doesn't remove the switch statements - it only introduces new arrays, as a potential for cache misses

This change essentially just rewrites existing code in a different way, with no clear benefit.
Like I mentioned in previous PRs, significant changes will only be accepted if they bring a significant improvement.

@turol
Copy link
Contributor Author

turol commented Sep 2, 2024

It doesn't remove the switches but it does make them somewhat more predictable. This should help the branch predictor.

The code is pretty well optimized already so to me 1-2% speedups are significant.

@vlutas
Copy link
Collaborator

vlutas commented Sep 2, 2024

Unfortunately, the improvement is nowhere nearly enough to justify such a significant change.

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.

3 participants