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

reduce op count for isinteger, add tests #14569

Merged
merged 1 commit into from
Jan 30, 2016
Merged

reduce op count for isinteger, add tests #14569

merged 1 commit into from
Jan 30, 2016

Conversation

simonbyrne
Copy link
Contributor

Okay, not the biggest optimisation in the world, but we reduce

  %1 = call double @llvm.trunc.f64(double %0)
  %2 = fcmp oeq double %1, %0
  %3 = fsub double %0, %0
  %4 = fcmp oeq double %3, 0.000000e+00
  %5 = and i1 %2, %4
  ret i1 %5

to

  %1 = call double @llvm.trunc.f64(double %0)
  %2 = fsub double %1, %0
  %3 = fcmp oeq double %2, 0.000000e+00
  ret i1 %3

@simonbyrne simonbyrne added the maths Mathematical functions label Jan 6, 2016
@StefanKarpinski
Copy link
Sponsor Member

No, this is great. I'd love to audit all of the simple numeric functions in base and make sure that they generate sane code and see if each one can be made any more efficient.

@simonbyrne
Copy link
Contributor Author

There probably won't be any real performance benefit until we enable new LLVM, as that should hopefully inline the trunc call using SSE instructions.

@yuyichao
Copy link
Contributor

yuyichao commented Jan 6, 2016

This seems to have 12-13% performance improvement with LLVM 3.7 =)

@jakebolewski
Copy link
Member

It would be great if we could begin to add benchmarks for this stuff. cc @jrevels

@jakebolewski jakebolewski added the potential benchmark Could make a good benchmark in BaseBenchmarks label Jan 6, 2016
@StefanKarpinski
Copy link
Sponsor Member

I wonder if benchmarking is really the best way to test for this kind of thing. Maybe checking for specific LLVM assembly? Of course, that doesn't address regressions in LLVM codegen, so maybe both.

@tkelman
Copy link
Contributor

tkelman commented Jan 7, 2016

LGTM, I like the test additions.

@kshyatt kshyatt added the performance Must go faster label Jan 7, 2016
jrevels added a commit to JuliaCI/BaseBenchmarks.jl that referenced this pull request Jan 28, 2016
@jrevels jrevels removed the potential benchmark Could make a good benchmark in BaseBenchmarks label Jan 28, 2016
hayd added a commit that referenced this pull request Jan 30, 2016
reduce op count for isinteger, add tests
@hayd hayd merged commit 7f205e5 into master Jan 30, 2016
@hayd
Copy link
Member

hayd commented Jan 30, 2016

@jrevels I guess the benchmark label was added for the idea of measuring the llvm (i.e. by LOC/instructions?)

@hayd hayd deleted the sb/isinteger branch January 30, 2016 03:21
@jrevels
Copy link
Member

jrevels commented Jan 30, 2016

My guess is that @jakebolewski added it because he thought we should perform actual timing measurements, so I added benchmarks that check various scalar predicates.

Adding tests that check the LLVM bitcode might be useful, but I'm not sure I'm knowledgable enough to write such a test effectively, and it's not currently something that gets covered by BaseBenchmarks.jl.

ScottPJones added a commit to ScottPJones/julia that referenced this pull request Jan 30, 2016
ScottPJones added a commit to ScottPJones/julia that referenced this pull request Jan 30, 2016
ScottPJones added a commit to ScottPJones/julia that referenced this pull request Jan 31, 2016
ScottPJones added a commit to ScottPJones/julia that referenced this pull request Jan 31, 2016
jishnub pushed a commit that referenced this pull request Jul 29, 2024
This is a very slight tweak to the implementation of
`isinteger(::AbstractFloat)` to use `iszero` rather than `== 0`. It
shouldn't make any difference with any of the built-in floating-point
types, but `iszero` might conceivably be faster for some user-defined
types.

I also added a comment to indicate why it's using `iszero(x - trunc(x))`
rather than `x == trunc(x)` (due to non-finite values); this code dates
back to #14569 in Julia 0.5.

---------

Co-authored-by: Sukera <[email protected]>
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
This is a very slight tweak to the implementation of
`isinteger(::AbstractFloat)` to use `iszero` rather than `== 0`. It
shouldn't make any difference with any of the built-in floating-point
types, but `iszero` might conceivably be faster for some user-defined
types.

I also added a comment to indicate why it's using `iszero(x - trunc(x))`
rather than `x == trunc(x)` (due to non-finite values); this code dates
back to JuliaLang#14569 in Julia 0.5.

---------

Co-authored-by: Sukera <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants