-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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] Add a TI_WITH_LLVM option #3507
Conversation
✔️ Deploy Preview for jovial-fermat-aa59dc canceled. 🔨 Explore the source changes: 8237e13 🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/6191b1151b5a0800079b1aad |
Co-authored-by: Bo Qiao <boqiao@taichi.graphics>
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.
Thanks. A quick grep into our cpp and h files shows 263 occurrences of endif
, many of which are small blocks. I feel like this may hinder readability in the long term.
I agree :(( The right thing to do here would be to:
I'm happy to create an issue to track this, but it would be a rather big effort. |
Indeed a big effort. Having a llvm folder in taichi/backend seems reasonable to me. This can also move some llvm-specific code from device.h/cpp. For now, i think this is fine. |
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.
LGTM!
This PR adds a TI_WITH_LLVM option, so that one can optionally disable compiling with LLVM. (For example, I'm trying to emscripten the entire taichi project, and LLVM doesn't like that).
TI_WITH_LLVM is defaulted to ON, so this PR won't affect any of our existing CI/CD/build tools.