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

Add config option for experimental LLVM targets #42643

Closed
wants to merge 1 commit into from

Conversation

tlively
Copy link
Contributor

@tlively tlively commented Jun 13, 2017

NVPTX and Hexagon LLVM targets are no longer built by default. Instead,
the developer has to opt in to building them by listing them in the new
experimental-targets option.

This change was suggested in #42571.
cc @eholk
r? @brson

NVPTX and Hexagon LLVM targets are no longer built by default. Instead,
the developer has to opt in to building them by listing them in the new
experimental-targets option.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@tlively
Copy link
Contributor Author

tlively commented Jun 13, 2017

This change is breaking for users of Hexagon and NVPTX, since they would have to update their config.toml. It also only works because LLVM's CMake setup does not differentiate between LLVM_TARGETS_TO_BUILD and LLVM_EXPERIMENTAL_TARGETS_TO_BUILD as far as I can tell. However, given that, this change may not be necessary, since the WebAssembly target in #42571 can simply be optionally added to the normal list of targets.

@hanna-kruppe
Copy link
Contributor

This change is breaking for users of Hexagon and NVPTX, since they would have to update their config.toml

They would have to start building rustc themselves, if they don't already (there's no reason for them to do so currently).

I'd like to hear some motivation for disabling these targets. The binary size increases were measured at least in the Hexagon case, and found to be neglegible. The build time shouldn't be a large factor either.

@tlively
Copy link
Contributor Author

tlively commented Jun 13, 2017

An alternative to this change (assuming removing NVPTX and Hexagon default support is still desired) would be to simply remove NVPTX and Hexagon from the defaults for the targets option. The only annoying thing about that would be that in order to build for the default targets in addition of Hexagon, for example, the user would have to manually list all the default targets and Hexagon instead of having to add only Hexagon.

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2017
@hanna-kruppe
Copy link
Contributor

Isn't the config.toml example supposed to contain a (commented out) up-to-date list of the default targets?

@tlively
Copy link
Contributor Author

tlively commented Jun 13, 2017

Isn't the config.toml example supposed to contain a (commented out) up-to-date list of the default targets?

Do you mean src/bootstrap/config.toml.example? This PR does update that file to reflect what would be the new defaults.

@hanna-kruppe
Copy link
Contributor

So if that file contains the default list of targets, I don't see how this is a burden at all:

The only annoying thing about that would be that in order to build for the default targets in addition of Hexagon, for example, the user would have to manually list all the default targets and Hexagon instead of having to add only Hexagon.

@tlively
Copy link
Contributor Author

tlively commented Jun 13, 2017

You're right, on second thought that's fairly trivial.

It seems to me that the best option would be to have our definition of "experimental" exactly match that of LLVM. That would mean the NVPTX and Hexagon would remain built by default, and this new configuration option would only be used for WebAssembly right now. In that case, this change can be made directly in #42571 and this PR can be closed.

@rkruppe, does that sound reasonable to you?

@hanna-kruppe
Copy link
Contributor

So the difference to the status quo would be that there's a way to pass -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD to CMake, and nothing else as far as non-wasm targets are concerned? Yes, that sounds good.

@tlively tlively closed this Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants