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

Use LLVM's byval instead of rewriting kernels. #16

Merged
merged 1 commit into from
May 15, 2020
Merged

Use LLVM's byval instead of rewriting kernels. #16

merged 1 commit into from
May 15, 2020

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Apr 27, 2020

No description provided.

@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #16 into master will decrease coverage by 1.43%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
- Coverage   68.16%   66.73%   -1.44%     
==========================================
  Files          18       18              
  Lines        1087     1034      -53     
==========================================
- Hits          741      690      -51     
+ Misses        346      344       -2     
Impacted Files Coverage Δ
src/irgen.jl 72.19% <100.00%> (-4.34%) ⬇️

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 7fd5e2e...398b8ac. Read the comment docs.

@maleadt
Copy link
Member Author

maleadt commented Apr 27, 2020

@jpsamaroo this triggers a bunch of address space-related issues with AMDGPUnative... Are those related to #13? Because the GCN tests in GPUCompiler do pass.

@vchuravy
Copy link
Member

No those are new https://github.com/llvm/llvm-project/blob/d359f2096850c68b708bc25a7baca4282945949f/llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp#L91-L100

By rewriting the kernel we have sofar tricked out the AA for AMDGPU.

@maleadt
Copy link
Member Author

maleadt commented May 6, 2020

Ah, this only used to fail when testing AMDGPUnative, not during GPUCompiler's GCN tests (maybe codegen tests covering the same functionality should be put here?), and AMD CI is down...

@jpsamaroo
Copy link
Member

AMDGPU CI should be back up. The issues with "pointer address space out of range" are due to Julia's optimization pipeline running the verifier at the beginning on debug builds, which #30 should hopefully fix for GCN.

@maleadt maleadt force-pushed the tb/byval branch 2 times, most recently from a634826 to 4f460f8 Compare May 15, 2020 07:21
@maleadt maleadt merged commit 5920663 into master May 15, 2020
@maleadt maleadt deleted the tb/byval branch May 15, 2020 07:51
@maleadt
Copy link
Member Author

maleadt commented May 20, 2020

Turns out there's a problem with this on Julia 1.4: NVPTX lowers byval by creating an alloca in the stack AS, which conflicts when passing a pointer in another address space: https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/lib/Target/NVPTX/NVPTXLowerArgs.cpp#L141-L168
Not a problem on 1.5, where we strip address spaces.

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