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

Enable support for 'concepts' in GCC 6 and later #3638

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Dec 16, 2017

GCC 6.x and higher supports 'concepts' as defined in the Concepts TS "C++ Extensions for concepts", ISO Technical Specification ISO/IEC TS 19217:2015.
With GCC 6.x, the -fconepts option enables support for concepts and defines __cpp_concepts to 201500.
With GCC 7.x, the -fconepts option enables support for concepts and defines __cpp_concepts to 201509, as dictated by the TS.

LLVM/clang and Intel ICC do not support 'concepts' yet.

GCC 6.x and higher supports 'concepts' as defined in the Concepts TS ["C++ Extensions for concepts", ISO Technical Specification ISO/IEC TS 19217:2015](https://www.iso.org/standard/64031.html).
With GCC 6.x, the `-fconepts` option enables support for concepts and defines `__cpp_concepts` to `201500`.
With GCC 7.x, the `-fconepts` option enables support for concepts and defines `__cpp_concepts` to `201509`, as dictated by the TS.

LLVM/clang and Intel ICC do not support 'concepts' yet.
@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 16, 2017

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 16, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25138/console

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fwyzard (Andrea Bocci) for branch IB/CMSSW_10_0_X/gcc630.

@cmsbuild, @smuzaffar, @gudrutis, @mrodozov can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3638/25138/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-3638/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 16 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2836825
  • DQMHistoTests: Total failures: 2286
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2834361
  • DQMHistoTests: Total skipped: 178
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.15000000007 KiB( 23 files compared)
  • Checked 113 log files, 9 edm output root files, 27 DQM output files

@smuzaffar smuzaffar merged commit 14d893f into cms-sw:IB/CMSSW_10_0_X/gcc630 Dec 18, 2017
@smuzaffar
Copy link
Contributor

@fwyzard , @davidlange6 , @Dr15Jones -fconcepts boost serialization . For now I am reverting it and we need to update build rules to drop flags which are not good for boost serialization.

>> Generating CondFormat Serialization code from header file src/CondFormats/CastorObjects/src/headers.h 
/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_0_X_2017-12-18-1100/src/CondFormats/Serialization/python/condformats_serialization_generate.py --output /build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_0_X_2017-12-18-1100/tmp/slc6_amd64_gcc630/src/CondFormats/CastorObjects/src/CondFormatsCastorObjects/a/Serialization.cc --package src/CondFormats/CastorObjects/ -- -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_0_X_2017-12-18-1100/src/CondFormats/CastorObjects/src -DGNU_GCC -D_GNU_SOURCE -DCMSSW_GIT_HASH="CMSSW_10_0_X_2017-12-18-1100" -DPROJECT_NAME="CMSSW" -DPROJECT_VERSION="CMSSW_10_0_X_2017-12-18-1100" -DTBB_USE_GLIBCXX_VERSION=60300 -DBOOST_SPIRIT_THREADSAFE -DPHOENIX_THREADSAFE -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_0_X_2017-12-18-1100/src -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/lcg/root/6.10.08-mmelna2/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/pcre/8.37-oenich/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/boost/1.63.0-mmelna/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/bz2lib/1.0.6/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/libuuid/2.22.2/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/tbb/2018_U1/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/xz/5.2.2-oenich/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/zlib-x86_64/1.2.11/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/md5/1.0.0/include -I/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/slc6_amd64_gcc630/external/tinyxml/2.5.3-mmelna/include -O2 -pthread -pipe -Werror=main -Werror=pointer-arith -Werror=overlength-strings -Wno-vla -Werror=overflow -std=c++17 -ftree-vectorize -fconcepts -Wstrict-overflow -Werror=array-bounds -Werror=format-contains-nul -Werror=type-limits -fvisibility-inlines-hidden -fno-math-errno --param vect-max-version-for-alias-checks=50 -Xassembler --compress-debug-sections -fno-crossjumping -msse3 -felide-constructors -fmessage-length=0 -Wall -Wno-non-template-friend -Wno-long-long -Wreturn-type -Wunused -Wparentheses -Wno-deprecated -Werror=return-type -Werror=missing-braces -Werror=unused-value -Werror=address -Werror=format -Werror=sign-compare -Werror=write-strings -Werror=delete-non-virtual-dtor -Werror=maybe-uninitialized -Werror=strict-aliasing -Werror=narrowing -Werror=uninitialized -Werror=unused-but-set-variable -Werror=reorder -Werror=unused-variable -Werror=conversion-null -Werror=return-local-addr -Werror=switch -fdiagnostics-show-option -Wno-unused-local-typedefs -Wno-attributes -Wno-psabi -DBOOST_DISABLE_ASSERTS -Wno-error=unused-variable -fdebug-prefix-map=/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w=/opt/cmssw -fdebug-prefix-map=/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw=/opt/cmssw -g
[2017-12-18 12:01:44,790] ERROR: Diagnostic: [Error] unknown argument: '-fconcepts'
[2017-12-18 12:01:44,790] ERROR:    at line 0 in None
Traceback (most recent call last):
  File "/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_0_X_2017-12-18-1100/src/CondFormats/Serialization/python/condformats_serialization_generate.py", line 603, in <module>
    main()
  File "/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_0_X_2017-12-18-1100/src/CondFormats/Serialization/python/condformats_serialization_generate.py", line 600, in main
    SerializationCodeGenerator( scramFlags=args[1:] ).generate( opts.output )
  File "/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/5b412993b38b4c687f9c4cfd1fca9306/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_0_X_2017-12-18-1100/src/CondFormats/Serialization/python/condformats_serialization_generate.py", line 505, in __init__
    raise Exception('Please, resolve all errors before proceeding.')
Exception: Please, resolve all errors before proceeding.
  gmake: *** [tmp/slc6_amd64_gcc630/src/CondFormats/CastorObjects/src/CondFormatsCastorObjects/a/Serialization.cc] Error 1
 Selected class -> std::vector<CastorPedestal, std::allocator<CastorPedestal> > for ROOT: vector<CastorPedestal>

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 18, 2017 via email

@smuzaffar
Copy link
Contributor

this requires some non-trivial changes in build rules.

@davidlange6
Copy link
Contributor

davidlange6 commented Dec 18, 2017 via email

@Dr15Jones
Copy link

@fwyzard what was the use case for turning on an experimental C++ feature?

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 18, 2017

@davidlange6

i'm not sure I follow why the gcc flags should not be used?

because we use clang to build the serialisation libraries, see CondFormats/Serialization/python/condformats_serialization_generate.py.

@Dr15Jones

what was the use case for turning on an experimental C++ feature?

Well, I would say "to make use of it".
In particular, I'm working on an interface to the DQMStore's MonitorElements that is templated on the object it holds, rather than relying on internal consistency checks for every call to Fill().
Since the DQMStore supports only a limited set of scalars and ROOT objects, I would like to restrict the types available for template instantiation - and I'd rather do it without all the SFINAE complications.

While it may be experimental, is is a publish standard; so a more apt question would be why we are not using it yet.

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 18, 2017

Actually, can we fix once and for all the rules used for serialisation to pick up the llvm/clang flags, rather than the gcc ones ?

@smuzaffar

this requires some non-trivial changes in build rules.

I understand, but it seems the special build rules need to be adjusted every time the GCC flags change.
Would you consider it as an improvement to target for 2018 ?

Also, why did the integration test not pick up the problem ?

@davidlange6
Copy link
Contributor

davidlange6 commented Dec 18, 2017 via email

@smuzaffar
Copy link
Contributor

@fwyzard ,

Would you consider it as an improvement to target for 2018 ?

yes

Also, why did the integration test not pick up the problem ?

For now, gcc-toolfile changes do not trigger built of any packages (actually it should rebuild full release). I will fix this this week.

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 18, 2017

ha - didn't know that. sounds like a fix for @ggovi to help with.

One improvement would be to fix CondFormats/Serialization/python/condformats_serialization_generate.py to call

scram b echo_CondFormatsCommon_CXXFLAGS COMPILER=llvm

rather than

scram b echo_CondFormatsCommon_CXXFLAGS

to get the clang-friendly flags.

However when we build a release SCRAM passes the compiler options directly, to avoid the 30-40 calls to discover the flags (once per CondFormats package) and thus be faster.
So here we also need to "teach" scram to use the llvm flags no matter what the current compiler is.

@Dr15Jones
Copy link

While it may be experimental, is is a publish standard; so a more apt question would be why we are not using it yet.

It was voted into the C++ 20 standard, however many compilers do not support it. GCC experimentally supports it but who knows how buggy or inefficient (both compile and run time) it is.

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 18, 2017

It was voted into the C++ 20 standard, however many compilers do not support it. GCC experimentally supports it but who knows how buggy or inefficient (both compile and run time) it is.

I am not talking about the C++20 standard, but about the Technical Specification (which means that, yes, it is experimental).

Interesting that you consider "buggy or inefficient" something you have not tried.
However, if you have any reason to believe it may be problematic, can we have it in the "devel" or "next" branch ?

@davidlange6
Copy link
Contributor

davidlange6 commented Dec 18, 2017 via email

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 18, 2017

Fair point - so, can we try it in the "next" branch, and see what it does on our large software stack ?

@Dr15Jones
Copy link

I would be OK with trying this on DEVEL

@davidlange6
Copy link
Contributor

davidlange6 commented Dec 18, 2017 via email

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 18, 2017

Since we need anyway to keep our code compiling under clang and maybe intel, we can wrap the concept-related stuff with

#if __cpp_concepts >= 201507
// concept-enabled stuff go here
#else
// non-concept fall back goes here
#endif

For example (here I use 201500 because gcc 6.3 has that)

#if __cpp_concepts >= 201500
template<typename T>
concept bool CopyConstructibleAndAssignable() {
  return std::is_copy_constructible<T>::value and std::is_copy_assignable<T>::value;
}
#else
#define CopyConstructibleAndAssignable typename
#endif

template <CopyConstructibleAndAssignable T>
class Container {
...
};

or (more intrusive)

#if __cpp_concepts >= 201500
template<typename T>
concept bool CopyConstructibleAndAssignable() {
  return std::is_copy_constructible<T>::value and std::is_copy_assignable<T>::value;
}
#endif

#if __cpp_concepts >= 201500
template <CopyConstructibleAndAssignable T>
#else
template <typename T>
#endif
class Container {
...
};

@davidlt
Copy link
Contributor

davidlt commented Dec 18, 2017

The original "Concepts" idea IIRC failed (years ago) and later "Concepts Lite" was pursued. I think, the latest Concept work was approved for C++20. Some (short, 2 pages) proposal:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0606r0.pdf
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0592r0.html

If enabling Concepts TS does not affect C++ ABI (probably it doesn't), then enabling it should not be a problem. This also helps to iron out bugs within GCC toolchain and provide further feedback to C++ committee meetings (that CERN attends).

The only request would be, that if code is broken due to Concepts implementation change within GCC then this should be fixed ASAP. Looks like Concepts might be already a bit different between Concepts TS (2015) and C++20 draft (now).

@fwyzard fwyzard deleted the IB/CMSSW_10_0_X/gcc630_enable_concepts branch February 16, 2018 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants