Page MenuHomePhabricator

test-release.sh: Add -openmp option.
ClosedPublic

Authored by hans on Jul 24 2015, 1:34 PM.

Details

Summary

This adds an -openmp option (off by default) for building the OpenMP run-time.

The run-time is still not well integrated with the regular LLVM build, but this allows building the run-time "on the side".

I'm thinking that we could provide separate pre-builts of the run-time (e.g. for x86_64 and Mac) at http://llvm.org/releases/download.html as a convenience for users who wish to experiment with Clang's OpenMP run-time.

Let me know what you think.

Diff Detail

Repository
rL LLVM

Event Timeline

hans updated this revision to Diff 30601.Jul 24 2015, 1:34 PM
hans retitled this revision from to test-release.sh: Add -openmp option..
hans updated this object.
hans added reviewers: jlpeyton, rengolin.
hans added subscribers: llvm-commits, openmp-commits.
rengolin edited edge metadata.Jul 28 2015, 4:44 AM

Hi Hans,

Sorry, this fell out of my radar. I think it's an important addition, I just think it should be part of the main build and behave like the others. I have left some comments inline.

Also, about the tar ball, are we going to start uploading both of them? Or is it maybe better to bundle the OpenMP tar inside the LLVM+Clang one? If we make this part of the main build, it may be easier to bundle all together.

Either way is fine for me, but would be good if all target testers / code owners would agree on one for a reason. :)

cheers,
--renato

utils/release/test-release.sh
440 ↗(On Diff #30601)

"set -x" considered harmful. :)

442 ↗(On Diff #30601)

This better stay in PhaseM/Release/... like the rest.

446 ↗(On Diff #30601)

Hasn't this moved out of /usr/local?

454 ↗(On Diff #30601)

Better not to override a global variable with the same name, it's confusing.

Call it OpenMPPackage or something.

455 ↗(On Diff #30601)

I think this should be inside the main build process, so that we use the same standard openmp-3.7.0-install and then rename / tar like llvmCore-3.7.0-install, etc.

hans added a comment.Jul 28 2015, 5:49 PM

Hi Hans,

Sorry, this fell out of my radar. I think it's an important addition, I just think it should be part of the main build and behave like the others. I have left some comments inline.

Also, about the tar ball, are we going to start uploading both of them? Or is it maybe better to bundle the OpenMP tar inside the LLVM+Clang one? If we make this part of the main build, it may be easier to bundle all together.

Either way is fine for me, but would be good if all target testers / code owners would agree on one for a reason. :)

I'm hoping that for the next release, the openmp builds and tests as any other LLVM module, by simply putting it under tools/ or projects. Currently it builds on the side and I'm not sure how to test it.

We could of course still work around that in the script and bake it into the regular LLVM+Clang tarball. However, I'm kind of hesitant to do so this late in the release process when none of us have really tested it.

My plan was to let those testers who wish try to build it, and then we'd upload the tarballs on the side, as a convenience for those who want to experiment with the openmp support.

utils/release/test-release.sh
440 ↗(On Diff #30601)

Removed.

442 ↗(On Diff #30601)

Done.

446 ↗(On Diff #30601)

No, but we make sure the /usr/local prefix doesn't go in the tarball.

454 ↗(On Diff #30601)

Done.

455 ↗(On Diff #30601)

I've commented on that below. I'm not sure we're ready yet.

hans updated this revision to Diff 30881.Jul 28 2015, 5:49 PM
hans edited edge metadata.

Addressing Renato's comments.

rengolin accepted this revision.Jul 29 2015, 7:08 AM
rengolin edited edge metadata.

LGTM, but we better add some comment saying it's an experimental feature and not meant for official testing the release (which then should be done at every stage).

This revision is now accepted and ready to land.Jul 29 2015, 7:08 AM
This revision was automatically updated to reflect the committed changes.