[WIP] Concepts refactor

[trws]

I'm putting this here in-progress right now, the goal is to re-factor the concepts stuff into a series of detectors and type-traits that are more separable, portable and reliable.

[trws]

Ok, it's time to get some eyes on this, the current version appears to work portably and improves the camp test compile time for ICC from 40 seconds to 11 on rztopaz. I still want to get something in as a helper to use the new concepts as constraints on functions and types that let people get them as actual concepts when in that mode, but take a look, maybe see if I'm crazy...

@davidbeckingsale, @willkill, @ajkunen

Should be overall a compile time improvement for everything that uses raja, especially that uses kernel, somewhere between 10% and 4x depending on exactly what the benchmark does is what I've seen (4x for crazy tuple test of doom).

@willkill: hope you don't mind a ping, it touches a bunch of your concept code and if you have the cycles I'd love it if you'd glance over it.

[rhornung67]

@trws is there a RAJA branch that compiles with this branch of camp? I tried master and develop and it died immediately.

[trws]

Not yet, RAJA will need some work to run with this cleanly, it shouldn’t be much but I’ll do it shortly.

…

Sent from my iPhone On Nov 19, 2019, at 11:45 AM, Rich Hornung <notifications@github.com> wrote:  @trws<https://github.com/trws> is there a RAJA branch that compiles with this branch of camp? I tried master and develop and it died immediately. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#8?email_source=notifications&email_token=AAFBFNIAMVQA6SHXD5G32M3QUQX2ZA5CNFSM4JKVUR52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEPI6UA#issuecomment-555650896>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFBFNOCV5J5TKHZTVQSUK3QUQX2ZANCNFSM4JKVUR5Q>.

[rhornung67]

Thanks @trws. Let me know when it's ready. I have a few code teams willing to try it out and report their findings to us.

[corbett5]

Commenting so that I get notified when this gets merged.

[willkill07]

@trws -- I never got the ping on this because my GitHub name has 07 at the end 🤦

What work needs done to get the revised detector idiom working? I originally was applying the nonesuch version to my concepts testbed repo back in 2017

[trws]

🤦‍♂️ ugh, sorry, bonehead mistake on my part.

It's working in here as is (or was, hopefully still is) the main rework is to deal with two issues, an EDG issue that only crops up with some nasty template type dependent cases, and the fact we were accidentally using binary results in some tests that weren't subtitution failures so they didn't trigger sfinae. Basically this is the meat of the EDG workaround:

#define CAMP_DEF_DETECTOR(name, params, ...)                   \
  template <CAMP_UNQUOTE params, typename __Res = __VA_ARGS__> \
  using name = __Res

//usage
CAMP_DEF_DETECTOR(has_begin, (typename T), decltype(val<T>().begin()));

... enable_if<is_detected<has_begin, T>::value> ...;

The strange, but useful, thing here is that all the compilers that bug out when doing indirection through a templated type dependent on a type member and all kinds of funny cases like that get it right when the detector or concept itself has a substitution failure in a default parameter. The only other way I found to get this to work is what boost and a bunch of the others do, declaring explicit partial overloads for each of the detected and not cases, which takes more than twice as long to compile.

As to the other bit, since the way we were using it in RAJA was throwing out the actual resulting type, some of the conditions we had for things like connectors to the is_numeric traits weren't actually working. They would pass, but they wouldn't ever fail on some compilers and somehow we never tripped over it until one of the codes tried to use that in isolation. That's why this branch had a rework to preserve the type for detected and things like the detected_convertible helper and so-forth.

I need to dig back into this and get it ready for actual use again. Much of what's in here is tested and works fine but we held back on it waiting to figure out the plan for C++14 so it's gotten out of sync, and could probably use an interface rework, I'm not sure I love all the extra macros.

[trws]

One concrete example I'm reminded of for the second issue is cases like this:

template<typename T>
using ht_int = DefineConcept(has_type<int>(val<T>() + val<T>()));

//desugars to
template <typename... T>
constexpr std::true_type
valid_expr(T &&...);
template <typename T, typename U>
constexpr auto has_type(U &&) -> if_<std::is_same<T, U>, std::true_type>;
template<typename T>
using ht_int = decltype(valid_expr(has_type<int>(val<T>() + val<T>()));

Since valid_expr always returns true_type if the expression is valid, whether the value is true or not, ht_int or any like it actually only trigger failure when the addition fails to substitute, even if the has_type fails it gets silently eaten on both detection and requires.

[corbett5]

So I have a use case where I would like to detect the existence of methods in a protected base class that are made public in a derived class.

template< typename T >
class Base
{
public:
  T * data()
  { return nullptr; }
};

template< typename T >
class Derived : protected Base< T >
{
public:
  using Base< T >::data;
};

I have my own detection logic (which I'd like to replace when this gets merged) but it fails with NVCC on this use case. So I decided to give this a try to see if it worked. I defined a concept as follows

CAMP_DEF_REQUIREMENT_T( has_data_method, val<T>().data() );
CAMP_DEF_CONCEPT_T( HasDataMethod, detect< has_data_method, T >() );

It works just fine on Quartz but on Lassen with clang-upstream-2019.03.26 and cuda-10.1.243 it breaks. Even stranger the boolean value is correct at runtime but not at compile time. Here's a test I wrote

CAMP_DEF_REQUIREMENT_T( has_data_method, val<T>().data() );

CAMP_DEF_CONCEPT_T( HasDataMethod, detect< has_data_method, T >() );

template< typename T >
class Base
{
public:
  void data()
  { return; }
};

template< typename T >
class Derived : protected Base< T >
{
public:
  using Base< T >::data;
};

template< bool B >
struct BoolStruct
{
  static constexpr bool value = B;
};

TEST(foo, bar)
{
  static_assert( CAMP_REQ( HasDataMethod, std::string ), "" );
  static_assert( !CAMP_REQ( HasDataMethod, int ), "" );

  static_assert( CAMP_REQ( HasDataMethod, Base< int > ), "" );

#if !defined(__CUDACC__)
  static_assert( CAMP_REQ( HasDataMethod, Derived< int > ), "" );
#else
  // This fails and the inverse does too!
  // static_assert( CAMP_REQ( HasDataMethod, Derived< int > ), "" );
  // static_assert( !CAMP_REQ( HasDataMethod, Derived< int > ), "" );
#endif

  ASSERT_EQ( CAMP_REQ( HasDataMethod, Derived< int > ), true );

  // This fails with NVCC even though the above passed.
  constexpr bool hasDataMethodAtCompileTime = BoolStruct< CAMP_REQ( HasDataMethod, Derived< int > ) >::value;
  ASSERT_EQ( hasDataMethodAtCompileTime, true );
}

This has got to be a compiler bug but if you find a way to make it work that would be awesome.

[trws]

That is absolutely fascinating, thank you so much for sending the test code @corbett5, finding good tests for this stuff is really tricky. It probably wont, but just in case it matters, were you compiling in c++11 or c++14 mode?

[corbett5]

I was using c++14.

[trws]

Well, @corbett5, I have both good and bad news for you. The bad news is that you're completely right that the detection for that pattern is broken, and that even the most resilient version of detector I know of fails on it. It fails, in some fashion, on every EDG compiler I have access to, gcc <= 6.0, clang up to current, and msvc. I know of no other template instantiation bug so prevalent across compilers, which is perversely impressive. @willkill07 put together a great test on godbolt here so you can see what happens on four of them. We're exploring workarounds, but so far the only workaround we've found is that if you do a check on the type of the expression rather than the validity, the check works properly on clang. Unfortunately it's still broken on icc, msvc and nvcc.

The "good" news out of all of this is that the approach we're using seems to work as much as any of the approaches do, so we don't have to rework with a slower or more cumbersome one, but this specific case breaks most compilers regardless. I'm going to do at least a bit more digging here for a workaround, but detection of public methods that were protected in base classes is very, very broken at the moment.

[corbett5]

Wow, thanks @trws @willkill07 so much for digging into this!