This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Update experimental target error message
ClosedPublic

Authored by hintonda on Dec 14 2017, 11:34 PM.

Details

Summary

Update this error message indicate this test only ensures experimental
targets were passed via LLVM_EXPERIMENTAL_TARGETS_TO_BUILD.

Originally, this test validated all targets, but in r184923, it was moved
after the LLVMBUILDTOOL test, which also validates all targets, making
that part of the test redundant.

Event Timeline

hintonda created this revision.Dec 14 2017, 11:34 PM
asb added a comment.Dec 15 2017, 12:15 AM

I have the same concern with this as I described in the discussion around r320413.

With this change, I can directly specify an experimental backend using -DLLVM_TARGETS_TO_BUILD and it will "just work". I think there is value in requiring experimental backends to be specified separately with -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD. It means that anyone auditing the build configuration can see at a glance when an experimental backend is being built, which I think is important given the lack of stability guarantees for these backends. Did you intend that change in behaviour?

In D41273#956447, @asb wrote:

I have the same concern with this as I described in the discussion around r320413.

With this change, I can directly specify an experimental backend using -DLLVM_TARGETS_TO_BUILD and it will "just work". I think there is value in requiring experimental backends to be specified separately with -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD. It means that anyone auditing the build configuration can see at a glance when an experimental backend is being built, which I think is important given the lack of stability guarantees for these backends. Did you intend that change in behaviour?

If you find it useful, I'm happy to leave it in, but we should at least change the error message to reflect what's really going on, i.e., the target was found but not explicitly passed as experimental.

hintonda updated this revision to Diff 127158.Dec 15 2017, 10:46 AM
  • Restore target test, but update error message to reflect that the target is experimental and must be passed via LLVM_EXPERIMENTAL_TARGETS_TO_BUILD.
asb accepted this revision.Dec 17 2017, 12:56 PM

This seems fine, but would be best with a comment that indicates that llvm-build will have already errored out if a totally unrecognised target was given.

This revision is now accepted and ready to land.Dec 17 2017, 12:56 PM
hintonda retitled this revision from [cmake] Remove redundant check for Targets to build to [cmake] Update experimental target error message.Dec 18 2017, 11:11 AM
hintonda edited the summary of this revision. (Show Details)
hintonda updated this revision to Diff 127392.Dec 18 2017, 11:12 AM
hintonda edited the summary of this revision. (Show Details)
  • Add comment per Alex's suggestion.
This revision was automatically updated to reflect the committed changes.