This is an archive of the discontinued LLVM Phabricator instance.

Add option to diable Bitcode tests. Tests are enabled by default.
AbandonedPublic

Authored by asbirlea on Jul 26 2016, 3:46 PM.

Details

Summary

Add the cmake option to diable bitcode tests.

Diff Detail

Event Timeline

asbirlea updated this revision to Diff 65619.Jul 26 2016, 3:46 PM
asbirlea retitled this revision from to Add option to diable Bitcode tests. Tests are enabled by default..
asbirlea updated this object.
asbirlea updated this revision to Diff 65627.Jul 26 2016, 4:21 PM

Update commit message with rationale.

MatzeB edited edge metadata.Jul 26 2016, 4:26 PM

Thanks for looking into it, we should still find a way to only have this enabled by default in configuration that actually work. I'll take a look later how we could determine whether the compiler in use is a "mainline" clang compiler.

Agreed, I'll wait for your input.

Hey Alina, please take a look at https://reviews.llvm.org/D22843. The changes there allow you to specify a minimum clang version and only trigger for "Clang" but not "AppleClang". I did not add an AppleClang case because as far as I can see the last released version of apple clang does not parse the bitcode correctly (people can still enable bitcode tests on appleclang by setting the option of course).

asbirlea abandoned this revision.Jul 26 2016, 11:00 PM

Sounds good to me.
As Mehdi pointed out, the apple compiler will probably want to use these tests as well, so I'm guessing we'll revisit the flag when the released version parses the bitcode correctly.
In the meantime, he's right that the bitcode tests should be generated with a release version. I'll create a separate patch for the x86 tests and update the aarch64 patch.
[Abandoning this patch, as D22843 is the better alternative.]

mehdi_amini edited edge metadata.Jul 26 2016, 11:05 PM

Sounds good to me.
As Mehdi pointed out, the apple compiler will probably want to use these tests as well, so I'm guessing we'll revisit the flag when the released version parses the bitcode correctly.

Well not really, the problem will stay forever (at some point our internal compiler will read the 3.9 bitcode but there might be some 4.0 bitcode). Also we're not the only downstream user, and other companies will have the same issue.

The approach @MatzeB took is a different philosophy from the one I was suggesting, basically he implemented opt-in while I was suggesting opt-out: an out-of-tree clang will have to explicitly pass the flag to get the tests enabled.

Either way (opt-in or opt-out) are fine to me, but I don't think we need to have code to handle every proprietary version system in the test-suite config upstream.

Ah, now I see your point about reading older bitcode. There will probably be a cmake option or different bitcode folders depending on the version. The check in the cmake file checking the version will still need to match at the time of adding bitcode 4.0 if it's mixed with the 3.9, or all tests be updated to the latest version.
The opt-in vs opt-out option still remains and honestly either option is fine with me. I do think the cmake flags make both options easier for out-of-tree compilers to set testing preferences.

Sounds good to me.
As Mehdi pointed out, the apple compiler will probably want to use these tests as well, so I'm guessing we'll revisit the flag when the released version parses the bitcode correctly.

Well not really, the problem will stay forever (at some point our internal compiler will read the 3.9 bitcode but there might be some 4.0 bitcode). Also we're not the only downstream user, and other companies will have the same issue.

We really need a consistent story here that is not just "Use todays ToT clang compiler", that is not how the test-suite is designed (just imagine bisecting over a larger range of commits). We should have some default versioning checks that to the best of our knowledge only enable configuration that work. If there are ever newer compilers that don't read the bitcode anymore we have to extend the checks we have.

The approach @MatzeB took is a different philosophy from the one I was suggesting, basically he implemented opt-in while I was suggesting opt-out: an out-of-tree clang will have to explicitly pass the flag to get the tests enabled.

Either way (opt-in or opt-out) are fine to me, but I don't think we need to have code to handle every proprietary version system in the test-suite config upstream.

I don't really understand the problem here. This is not about shutting out apple clang, this is about not having the test-suite fail out of the box which is a bad user experience.
If out of tree clang versions need an extra flag to enable the tests that is a price we should pay to not have the test-suite fail on other people who may have no clue about bitcode tests and versioning issues.

@mehdi_amini, @MatzeB: Looking into updating the tests with the 3.9 release, there is no difference from the current ones.
(other than the !0 attribute showing clang version 3.9.0 on both but a different trunk revision).
I don't think it's worth it to update them right now.