-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Enable LLVM NVPTX back-end #19323
Enable LLVM NVPTX back-end #19323
Conversation
@@ -481,6 +485,12 @@ $(eval $(call LLVM_PATCH,llvm-arm-fix-prel31)) | |||
$(eval $(call LLVM_PATCH,llvm-D25865-cmakeshlib)) | |||
endif # LLVM_VER | |||
|
|||
ifeq ($(LLVM_VER_SHORT),3.9) |
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.
why is this separate from the above list?
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.
Put it there in my branch to avoid merge conflicts, will change.
@@ -78,7 +78,11 @@ LLVM_CFLAGS += $(CFLAGS) | |||
LLVM_CXXFLAGS += $(CXXFLAGS) | |||
LLVM_CPPFLAGS += $(CPPFLAGS) | |||
LLVM_LDFLAGS += $(LDFLAGS) | |||
LLVM_TARGETS := host | |||
ifeq ($(LLVM_USE_CMAKE),1) | |||
LLVM_TARGETS := host;NVPTX |
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.
we don't usually indent this kind of thing in makefiles
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.
Fair enough.
will need to be tested on the buildbots to make sure it builds everywhere, if it's going to be enabled on all platforms by default |
b103efd
to
ef5b0e2
Compare
Travis is happy. Any other configurations to manually test this on? |
all of the buildbots |
Right, but those only run on |
Perhaps @tkelman or @staticfloat can share the creds with you for kicking off builds. |
I triggered a set of builds of this branch, they're running now. Something unrelated is wrong on the mac buildbot, it's having python issues with building the docs. |
The windows bots are acting up:
Not sure how that could happen, it seems like LLVM 3.7 doesn't build if the NVPTX target is enabled? |
I guess things would be easier if were already using llvm 3.9. |
It does seem that llvm 3.9 is awfully close - but are we going to move to it for 0.6? If not, since it is late in the cycle, should we merge this? Having CUDANative.jl work with Julia 0.6 out of the box would be a nice feature to add, like we did ARM and POWER for 0.5. |
that's a no, we shouldn't merge this without upgrading llvm |
ef5b0e2
to
94c8866
Compare
Actually we should probably get the LLVM patches here onto master before we upgrade, so they go into the homebrew and appveyor binaries (I'll need to rebuild the latter) and the CI build matches what you get in a source build. |
OK, I can split those off. I just rebased this branch on top of your LLVM 3.9 update PR to first see if NVPTX will build correctly on Windows. |
Sure, worth testing. Do you have a login to the buildbots, or need me to fire that off? (edit: running now) |
Shouldn't we also include the NVPTX back-end in the homebrew/appveyor binaries, if we want to ship that as part of Julia 0.6 @ LLVM 3.9? To do so, we could shield the That said, unless I still manage to create a PR for packaging an additional 'dev tools' tarball (with |
Sounds like it wouldn't add all that much value to turn on by default as long as this is the case? |
Ah yes, this will fail until JuliaCI/julia-buildbot#54 is resolved of course.
|
Yes, if it builds on all platforms, this is a great idea. |
Let's do that then - just your commit, modified to only build the nvptx backend when using llvm >= 3.9. This branch seems to be working everywhere, but let's rearrange it so we can merge it independently. I'll need to rebase #19678 on top of #19681 anyway, and it'll have to wait for the homebrew formula and some benchmarking. |
8358eee
to
0a55584
Compare
|
0a55584
to
1658320
Compare
LLVM_TARGET_STRING := $(subst $(SPACE),;,$(LLVM_TARGETS)) | ||
else | ||
COMMA := , | ||
LLVM_TARGET_STRING := $(subst $(SPACE),$(COMMA),$(LLVM_TARGETS)) |
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.
do we need to worry about this case? it's not possible to use configure with 3.9 or newer, so I'd simplify this as something like
ifeq ($(LLVM_VER_SHORT),$(filter $(LLVM_VER_SHORT),3.3 3.4 3.5 3.6 3.7 3.8))
LLVM_TARGET_STRING := host
else
LLVM_TARGET_STRING := host;NVPTX
endif
if that works
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 figured I'd keep it a little more extensible, but sure that works too. I'll update the PR.
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.
people can always override LLVM_TARGETS
(or LLVM_TARGET_STRING
or whatever) in their Make.user
, right?
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.
pretty much, yeah.
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.
then I guess we could keep it called LLVM_TARGETS
just for minimal disruption to anyone who might already be doing this?
1658320
to
eaf990f
Compare
Use of the NVPTX back-end is only tested on LLVM 3.9, in combination with the CUDAnative.jl package.
eaf990f
to
706c764
Compare
@@ -73,17 +73,22 @@ LLVM_LIBCXX_TAR:=$(SRCDIR)/srccache/libcxx-$(LLVM_TAR_EXT) | |||
endif | |||
endif # LLVM_VER != svn | |||
|
|||
# Figure out which targets to build | |||
ifeq ($(LLVM_VER_SHORT),$(filter $(LLVM_VER_SHORT),3.3 3.4 3.5 3.6 3.7 3.8)) |
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.
Does this condition evaluate to true when LLVM_VER_SHORT is svn ?
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 shouldn't.
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.
How do you enable the NVPTX back-end for svn builds ?
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 should be enabled since the false branch enables nvptx backend............
(Assumed) non-controversial parts of #19302, enables the LLVM NVPTX back-end (1 MB, 3% file size increase for
libLLVM
) and adds some crucial patches related to JuliaGPU/CUDAnative.jl#13 (all of them merged already):