-
Notifications
You must be signed in to change notification settings - Fork 688
Remove trivial ecma_number arithmetic functions #2123
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
Remove trivial ecma_number arithmetic functions #2123
Conversation
|
I don't see any improvement here. This is just a noise. I think these inlining are done by compiler automatically, because the binary size did not increase. |
|
@LaszloLango I've updated the result with the run time standard deviation and highlighted the tests and results where the gain is magnitude greater then the std deviation. That's why I think it's more then noise. |
|
Also I've compared the two binaries (with patch and without patch) and there is difference between them. It's means inlining these functions has real effect. |
| * @return number - result of addition. | ||
| */ | ||
| ecma_number_t | ||
| inline ecma_number_t __attr_always_inline___ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just remove these functions.
e8153f2 to
b4bcc77
Compare
|
@zherczeg Your suggestion have been applied! |
b4bcc77 to
4d3bbef
Compare
jerry-core/vm/vm.c
Outdated
| ecma_number_t new_value = ecma_number_add (ecma_get_float_from_value (left_value), | ||
| ecma_get_number_from_value (right_value)); | ||
| ecma_number_t new_value = ecma_get_float_from_value (left_value) + | ||
| ecma_get_number_from_value (right_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not nice alignment. What is the difference between float value and number value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number value not specifies that the stored value is float or integer. Unfortunately it is not a nice alignment, but this is what vera++ requires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use () brackets to help aligning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked! Thanks for your advice!
The affected function calls have been replaced with the appropriate arithmetic operands. JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu
4d3bbef to
f1fcbf8
Compare
zherczeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LaszloLango
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The affected function calls have been replaced with the appropriate arithmetic operands.
JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu