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

[Feature Request]: expose LLVM_TARGETS_TO_BUILD in third-party/llvm/Makefile so builder can override. #26203

Closed
hpcpony opened this issue Nov 4, 2024 · 5 comments · Fixed by #26341

Comments

@hpcpony
Copy link

hpcpony commented Nov 4, 2024

Summary of Feature

Description:
Would it be reasonable to change third-party/llvm/Makefile so that if a user has LLVM_TARGETS_TO_BUILD (or if you want to call it something else that's fine) defined then that is used as opposed to the current default ("host;X86;AArch64;NVPTX;AMDGPU"). If no environment value is defined then default to everything: . Typically I only need "host" or "host" + one of the GPUs and I don't really need to build everything else.

Is this issue currently blocking your progress?
no

@mppf
Copy link
Member

mppf commented Nov 6, 2024

Sounds like a good idea. @hpcpony I think this could be a relatively small change to third-party/llvm/Makefile. Would you be up for making a PR to do it?

@hpcpony
Copy link
Author

hpcpony commented Nov 15, 2024

Sorry, I've been away...

I don't know how to do all the github stuff, nor have resources to test on other than one system but here's the diff of what I do locally to third-party/llvm/Makefile that seems to be working for me:

4a5,8
> ifndef CHPL_LLVM_TARGETS_TO_BUILD
> export CHPL_LLVM_TARGETS_TO_BUILD="host;X86;AArch64;NVPTX;AMDGPU"
> endif
> 
106c110
< 	    -DLLVM_TARGETS_TO_BUILD="host;X86;AArch64;NVPTX;AMDGPU" \
---
> 	    -DLLVM_TARGETS_TO_BUILD=$(CHPL_LLVM_TARGETS_TO_BUILD) \

@jabraham17
Copy link
Member

I found myself really wanting this improvement today and tried the diff. I made a small adjustment so that something like export CHPL_LLVM_TARGETS_TO_BUILD='host;NVPTX' gets properly quoted, but this was a nice time saver.

diff --git a/third-party/llvm/Makefile b/third-party/llvm/Makefile
index 5db91cbd182..0b982d1eb9e 100644
--- a/third-party/llvm/Makefile
+++ b/third-party/llvm/Makefile
@@ -12,6 +12,12 @@ else
   CHPL_LLVM_DEBUG := -DCMAKE_BUILD_TYPE=Release
 endif
 
+ifndef CHPL_LLVM_TARGETS_TO_BUILD
+  CHPL_LLVM_TARGETS_TO_BUILD := "host;X86;AArch64;NVPTX;AMDGPU"
+else
+  CHPL_LLVM_TARGETS_TO_BUILD := "$(CHPL_LLVM_TARGETS_TO_BUILD)"
+endif
+
 # activate LLVM asserts if it's from `make ASSERTS=1`
 # or if CHPL_LLVM_DEVELOPER is set
 CHPL_LLVM_ASSERTS := 0
@@ -105,7 +111,7 @@ llvm-config: FORCE
 	    -DCLANG_ENABLE_ARCMT=0 \
 	    -DCLANG_ANALYZER_ENABLE_Z3_SOLVER=0 \
 	    -DCLANG_ENABLE_STATIC_ANALYZER=0 \
-	    -DLLVM_TARGETS_TO_BUILD="host;X86;AArch64;NVPTX;AMDGPU" \
+	    -DLLVM_TARGETS_TO_BUILD=$(CHPL_LLVM_TARGETS_TO_BUILD) \
 	    -DLLVM_INSTALL_UTILS=ON \
 	    $(CHPL_LLVM_NO_TESTS) \
 	    $(CHPL_LLVM_ENABLE_PROJECTS) \

@bradcray
Copy link
Member

bradcray commented Dec 3, 2024

@jabraham17 : As an advocate, would you be up for making a PR for this since @hpcpony seems (understandably) disinclined to go through the GitHub learning curve?

@jabraham17
Copy link
Member

Yup, done in #26341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants