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

LLVM 12 updates. #86

Merged
merged 2 commits into from
May 12, 2021
Merged

LLVM 12 updates. #86

merged 2 commits into from
May 12, 2021

Conversation

chriselrod
Copy link
Contributor

@chriselrod chriselrod commented May 12, 2021

I haven't actually tested on LLVM 12.

@@ -775,8 +775,9 @@ for (fs, c) in zip([HORZ_REDUCTION_OPS_FLOAT, HORZ_REDUCTION_OPS_INT],
[FloatingTypes, IntegerTypes])
for f in fs
f_red = Symbol("reduce_", f)
ff_base = Base.libllvm_version < v"12" ? "experimental.vector.reduce." : "vector.reduce."
Copy link
Owner

Choose a reason for hiding this comment

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

This could be declared outside the function (with a different name).

Copy link
Contributor Author

@chriselrod chriselrod May 12, 2021

Choose a reason for hiding this comment

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

Alternatively, I could replace this with

ff_base = string(Base.libllvm_version < v"12" ? "experimental.vector.reduce." : "vector.reduce.", $(QuoteNode(f)))

For the negligible improvement in compilation time from not concatenating inside the @generated function body.

Please let me know which you prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer code that is the easiest to read. Compile time differences will be negligible, and there will not be any run time difference. Since there is an existing, similar check outside the function, I thought it would be good to either have all version-dependent checks in the same place. Anything else I leave to you.

for (f, neutral) in [(:fadd, "0.0"), (:fmul, "1.0")]
f_red = Symbol("reduce_", f)
ff_base = Base.libllvm_version < v"12" ? "experimental.vector.reduce.$horz_reduction_version" : "vector.reduce.$horz_reduction_version"
Copy link
Owner

Choose a reason for hiding this comment

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

This could be declared outside the function (with a different name), next to horz_reduction_version.

@eschnett
Copy link
Owner

Is there a way to test with LLVM 12?

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2021

Codecov Report

Merging #86 (f5fee17) into master (152aaae) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #86   +/-   ##
=======================================
  Coverage   93.28%   93.28%           
=======================================
  Files           5        5           
  Lines         700      700           
=======================================
  Hits          653      653           
  Misses         47       47           
Impacted Files Coverage Δ
src/LLVM_intrinsics.jl 97.92% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 152aaae...f5fee17. Read the comment docs.

@vchuravy
Copy link
Collaborator

Is there a way to test with LLVM 12?

You can check out JuliaLang/julia#40774 which has pre-built artifacts for LLVM

@chriselrod
Copy link
Contributor Author

I tried it locally:

     Testing SIMD tests passed

julia> Base.libllvm_version
v"12.0.0"

julia> versioninfo()
Julia Version 1.7.0-DEV.1093
Commit 0942e35fbd* (2021-05-12 15:45 UTC)
Platform Info:
  OS: Linux (x86_64-generic-linux)
  CPU: Intel(R) Core(TM) i9-7900X CPU @ 3.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.0 (ORCJIT, skylake-avx512)
Environment:
  JULIA_NUM_THREADS = auto

@eschnett eschnett merged commit 8c5bac8 into eschnett:master May 12, 2021
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.

5 participants