Page MenuHomePhabricator

[test-suite] A number of cmake configuration fixes for External/SPEC.
ClosedPublic

Authored by kristof.beyls on Jan 28 2016, 11:55 AM.

Details

Summary

These are the changes I had to make to make the spec 2000 and spec 2006 benchmark
compile and run correctly using the new cmake/lit-framework in the test-suite,on the following platforms, that I used for testing:

  • Ubuntu 12.04LTS x86_64, gcc 4.7
  • Ubuntu 12.04LTS x86_64, clang 3.5
  • Debian linux AArch64 v8.0, clang 3.7
  • Debian linux AArch32 v8.0, clang 3.7
  • OSX Yosemite x86_64, Xcode clang 7.0.0

Diff Detail

Repository
rL LLVM

Event Timeline

kristof.beyls retitled this revision from to [test-suite] A number of cmake configuration fixes for External/SPEC..
kristof.beyls updated this object.
kristof.beyls added reviewers: MatzeB, jmolloy, mcrosier.
kristof.beyls added a subscriber: llvm-commits.
MatzeB edited edge metadata.Jan 28 2016, 1:05 PM

Thanks for working on this, this is looking good. At a first glance the changes look good (some comments below). I'll do a testrun later to see whether it still works on our platform with these changes.

External/SPEC/CINT2000/176.gcc/CMakeLists.txt
7–9 ↗(On Diff #46302)

I think we can add that flag unconditionally.

External/SPEC/CINT2000/252.eon/CMakeLists.txt
8–10 ↗(On Diff #46302)

Why do you need to disable this specific warning? I would assume we get many other warnings which are not disabled as well.
Also you can write it the if like this:

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
External/SPEC/CINT2000/253.perlbmk/CMakeLists.txt
47–49 ↗(On Diff #46302)

We can add that flag unconditionally.

MatzeB added inline comments.Jan 28 2016, 7:37 PM
External/SPEC/CINT2000/252.eon/CMakeLists.txt
5–6 ↗(On Diff #46302)

Without the -stdlib=libstdc++ the benchmark does not compile for me because it cannot find <iostream.h>. I'm surprised that worked with clang/OS X for you, maybe it is because you have an older version than me. I guess we need something like a global LIBSTDCXX_AVAILABLE variable that defaults to true if APPLE is set?

kristof.beyls added inline comments.Jan 29 2016, 7:32 AM
External/SPEC/CINT2000/176.gcc/CMakeLists.txt
7–9 ↗(On Diff #46302)

Ah yes, indeed.
I tried to make sure that the cmake files allow to also run with gcc instead of clang, but -std=gnu89 is indeed also a gcc command line option with the same semantics.

External/SPEC/CINT2000/252.eon/CMakeLists.txt
5–6 ↗(On Diff #46302)

It turns out my copy of spec2000 has been modified to work around the iostream.h issue, by adding an iostream.h in the eon source code directory, which just does:
#include <iostream>
using namespace std;

I really dislike changing the source code of the benchmark, but I also don't really like being stuck to use libstdc++ and not libc++.
I think that the ideal work-around would be to add a workaround like "#include <iostream>\nusing namespace std;", but without having to change the SPEC source code. I'm not sure what the least-convoluted way would be to achieve something like that from just adapting the 252.eon/CMakeLists.txt file. I'll think a bit more about this next week, if no one else comes up with a reasonable suggestion by then.

8–10 ↗(On Diff #46302)

Strange, I thought I needed it to disable an error message, but I can't reproduce it. Let's remove this from my patch.

External/SPEC/CINT2000/253.perlbmk/CMakeLists.txt
47–49 ↗(On Diff #46302)

agreed, will change that in the next version of this patch.

MatzeB added inline comments.Jan 29 2016, 10:50 AM
External/SPEC/CINT2000/252.eon/CMakeLists.txt
5–6 ↗(On Diff #46302)

Well it's clearly ancient C++ code in the benchmark so working around that a bit is fair IMO. Anyway if it is indeed just iostream.h then I'd suggest we place a file like you described into the directory where the CMakeLists.txt is and add a -I flag for that. We already did similar hacks if you look at (test-suite/SPEC/CINT2006/483.xalancbmk/ppc/endian.h and test-suite/SPEC/CINT95/124.m88ksim/builtins.h)

MatzeB added a comment.Feb 2 2016, 1:53 PM

I just pushed a r259581 which provides an iostream.h and fstream.h in the benchmark directory that just includes iostream and fstream and removes the -stdlib=libstdc++.

I just pushed a r259581 which provides an iostream.h and fstream.h in the benchmark directory that just includes iostream and fstream and removes the -stdlib=libstdc++.

Hi Matthias,

I think I've just found out why we've been seeing differences in our runs.
It turns out that spec have made a number of dot-releases of both SPEC2000 and SPEC2006.
I've been using the latest release, i.e. SPEC2000 v1.3 (see http://spec.org/cpu2000/) and SPEC2006 v1.2 (see http://spec.org/cpu2006/).

The issue for 252.eon using iostream.h and fstream.h has been fixed/worked around in the SPEC2000 v1.3 source code (see http://spec.org/cpu2000/docs/changes_in_V1.3.html, search for 252.eon).
Next to the issue in 252.eon, I also see at least an issue with 403.gcc in SPEC2006: the input data files have been renamed, and therefore the inputs as recorded in the CMakefiles don't exist (see http://spec.org/cpu2006/Docs/changes-in-v1.2.html, search for 403.gcc for details).

So, I think before trying to fix the issues I'm seeing, we should first make a decision on which versions of spec2000 and spec2006 we'd want to support in the CMakefiles.
Would it be acceptable to only support spec2000 v1.3 and spec2006 v1.2 - i.e. the latest releases? If not, we'll probably need a few more conditionals in the CMakfiles, based on the spec release - e.g. for the changed names of the input files for 403.gcc.

What do you think?

Thanks,

Kristof

MatzeB accepted this revision.Feb 8 2016, 1:33 PM
MatzeB edited edge metadata.

I never gave a LGTM even though there is no problem with the patch anymore, independently of the spec revision discussion.

This revision is now accepted and ready to land.Feb 8 2016, 1:33 PM
This revision was automatically updated to reflect the committed changes.