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

Check variadic pattern arguments at compile time #356

Merged
merged 10 commits into from
May 30, 2017
8 changes: 8 additions & 0 deletions dash/include/dash/Exception.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
throw(excep_type(os.str())); \
} while(0)

#define DASH_ASSERT_ALWAYS(expr) do { \
if (!(expr)) { \
DASH_THROW(dash::exception::AssertionFailed, \
"Assertion failed: " \
<< " " << __FILE__ << ":" << __LINE__); \
}\
} while(0)

#if defined(DASH_ENABLE_ASSERTIONS)

#define DASH_ASSERT(expr) do { \
Expand Down
145 changes: 75 additions & 70 deletions dash/include/dash/pattern/internal/PatternArguments.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#ifndef DASH__INTERNAL__PATTERN_ARGUMENTS_H_
#define DASH__INTERNAL__PATTERN_ARGUMENTS_H_

#include <utility>

#include <dash/Types.h>
#include <dash/Team.h>
#include <dash/Dimensional.h>
Expand All @@ -25,7 +27,7 @@ template<
dim_t NumDimensions,
typename IndexType = dash::default_index_t>
class PatternArguments {
private:
public:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why public?

/// Derive size type from given signed index / ptrdiff type
typedef typename std::make_unsigned<IndexType>::type
SizeType;
Expand All @@ -51,12 +53,6 @@ class PatternArguments {
ViewSpec_t _viewspec;
/// Team containing all units to which pattern elements are mapped
Team * _team = nullptr;
/// Number of distribution specifying arguments in varargs
int _argc_dist = 0;
/// Number of size/extent specifying arguments in varargs
int _argc_size = 0;
/// Number of team specifying arguments in varargs
int _argc_team = 0;

public:
/**
Expand All @@ -71,27 +67,14 @@ class PatternArguments {
*/
template<typename ... Args>
PatternArguments(Args && ... args) {
static_assert(
sizeof...(Args) >= NumDimensions,
"Invalid number of arguments for PatternArguments");
// Parse argument list:
check_recurse<0>(std::forward<Args>(args)...);
// Validate number of arguments after parsing:
if (_argc_size > 0 && _argc_size != NumDimensions) {
DASH_THROW(
dash::exception::InvalidArgument,
"Invalid number of size arguments for BlockPattern(...), " <<
"expected " << NumDimensions << ", got " << _argc_size);
}
if (_argc_dist > 0 && _argc_dist != NumDimensions) {
DASH_THROW(
dash::exception::InvalidArgument,
"Invalid number of dist arguments for BlockPattern(...), " <<
"expected " << NumDimensions << ", got " << _argc_dist);
}
check_tile_constraints();
check<0, 0, 0>(std::forward<Args>(args)...);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meanwhile it is probably more a parse than a check.

}


public:


bool is_tiled() const {
for (auto d = 0; d < NumDimensions; ++d) {
if (_distspec[d].is_tiled()) {
Expand Down Expand Up @@ -122,71 +105,91 @@ class PatternArguments {

private:
/// BlockPattern matching for extent value of type IndexType.
template<int count>
void check(SizeType extent) {
template<int argc_size, int argc_dist, int has_team, typename ... Args>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some documentation would be nice for those parameters.

void check(SizeType extent, Args && ... args) {
static_assert(argc_size >= 0, "Cannot mix size and SizeSpec definition"
"in variadic pattern constructor!");
DASH_LOG_TRACE("PatternArguments.check(extent)", extent);
static_assert(argc_size < NumDimensions, "Number of size specifier exceeds"
"the number of dimensions in variadic pattern constructor!");
DASH_LOG_TRACE("PatternArguments.check(extent)", extent);
_argc_size++;
_sizespec.resize(count, extent);
_sizespec.resize(argc_size, extent);
check<argc_size+1, argc_dist, has_team>(std::forward<Args>(args)...);
}
/// BlockPattern matching for up to \c NumDimensions optional
/// parameters specifying the distribution pattern.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems to be shifted.

template<int count>
void check(const TeamSpec_t & teamSpec) {
template<int argc_size, int argc_dist, int has_team, typename ... Args>
void check(const TeamSpec_t & teamSpec, Args && ... args) {
static_assert(has_team == 0,
"Cannot mix Team and TeamSpec definition in variadic "
"pattern constructor!");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This limitation seems a little bit odd,
but I think this is depended on TeamSpec.

DASH_LOG_TRACE("PatternArguments.check(teamSpec)");
_argc_team++;
_teamspec = teamSpec;
check<argc_size, argc_dist, -1>(std::forward<Args>(args)...);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No Team is set in this case, it was probably broken before as well...

}
/// BlockPattern matching for one optional parameter specifying the
/// team.
template<int count>
void check(dash::Team & team) {
template<int argc_size, int argc_dist, int has_team, typename ... Args>
void check(dash::Team & team, Args && ... args) {
static_assert(!(has_team < 0),
"Cannot mix Team and TeamSpec definition in variadic "
"pattern constructor!");
static_assert(has_team == 0,
"Cannot specify Team twice in variadic "
"pattern constructor!");
DASH_LOG_TRACE("PatternArguments.check(team)");
if (_argc_team == 0) {
_team = &team;
_teamspec = TeamSpec_t(_distspec, team);
}
_team = &team;
check<argc_size, argc_dist, 1>(std::forward<Args>(args)...);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No TeamSpec is created in this case,
as it is dependend on _distspec it probably has to go in the final check() call.

}
/// BlockPattern matching for one optional parameter specifying the
/// size (extents).
template<int count>
void check(const SizeSpec_t & sizeSpec) {
template<int argc_size, int argc_dist, int has_team, typename ... Args>
void check(const SizeSpec_t & sizeSpec, Args && ... args) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it makes sense to move this function up,
so that the it is grouped with the function taking individual extents.

static_assert(argc_size == 0, "Cannot mix size and SizeSpec definition"
"in variadic pattern constructor!");
DASH_LOG_TRACE("PatternArguments.check(sizeSpec)");
_argc_size += NumDimensions;
_sizespec = sizeSpec;
check<-1, argc_dist, has_team>(std::forward<Args>(args)...);
}
/// BlockPattern matching for one optional parameter specifying the
/// distribution.
template<int count>
void check(const DistributionSpec_t & ds) {
template<int argc_size, int argc_dist, int has_team, typename ... Args>
void check(const DistributionSpec_t & ds, Args && ... args) {
static_assert(argc_dist == 0, "Cannot mix DistributionSpec and inidividual"
"distributions in variadic pattern constructor!");
DASH_LOG_TRACE("PatternArguments.check(distSpec)");
_argc_dist += NumDimensions;
_distspec = ds;
check<argc_size, -1, has_team>(std::forward<Args>(args)...);
}
/// BlockPattern matching for up to NumDimensions optional parameters
/// BlockPattern matching for up to \c NumDimensions optional parameters
/// specifying the distribution.
template<int count>
void check(const Distribution & ds) {
template<int argc_size, int argc_dist, int has_team, typename ... Args>
void check(const Distribution & ds, Args && ... args) {
static_assert(!(argc_dist < 0), "Cannot mix DistributionSpec and "
"inidividual distributions in variadic pattern constructor!");
static_assert(argc_dist < NumDimensions, "Number of distribution specifier"
" exceeds the number of dimensions in variadic pattern constructor!");
DASH_LOG_TRACE("PatternArguments.check(dist)");
_argc_dist++;
dim_t dim = count - NumDimensions;
_distspec[dim] = ds;
}
/// Isolates first argument and calls the appropriate check() function
/// on each argument via recursion on the argument list.
template<int count, typename T, typename ... Args>
void check_recurse(T && t, Args && ... args) {
DASH_LOG_TRACE("PatternArguments.check(args) ",
"count", count,
"argc", sizeof...(Args));
check<count>(std::forward<T>(t));
if (sizeof...(Args) > 0) {
check_recurse<count + 1>(std::forward<Args>(args)...);
}
_distspec[argc_dist] = ds;
check<argc_size, argc_dist+1, has_team>(std::forward<Args>(args)...);
}
/// Terminator function for recursive argument parsing
template<int count>
void check_recurse() {

/**
* Stop recursion when all arguments are processed.
*/
template<int argc_size, int argc_dist, int has_team>
void check() {
static_assert(!(argc_dist > 0 && argc_dist != NumDimensions),
"Incomplete distribution specification in "
"variadic pattern constructor!");

static_assert(!(argc_size > 0 && argc_size != NumDimensions),
"Incomplete size specification in "
"variadic pattern constructor!");

check_tile_constraints();
}

/// Check pattern constraints for tile
void check_tile_constraints() const {
bool has_tile = false;
Expand All @@ -197,20 +200,22 @@ class PatternArguments {
if (_distspec.dim(i).type != _distspec.dim(i+1).type)
invalid = true;
}
if (has_tile && invalid) {
DASH_THROW(dash::exception::InvalidArgument,
"Pattern arguments invalid: Mixed distribution types");
}
if (has_tile) {
if (invalid) {
DASH_THROW(dash::exception::InvalidArgument,
"Pattern arguments invalid: Mixed distribution types");
}

for (auto i = 0; i < NumDimensions; i++) {
assert(
DASH_ASSERT_ALWAYS(
_sizespec.extent(i) % (_distspec.dim(i).blocksz)
== 0);
}
}
}
};


} // namespace internal
} // namespace dash

Expand Down
21 changes: 20 additions & 1 deletion dash/test/pattern/BlockPatternTest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,16 @@ TEST_F(BlockPatternTest, SimpleConstructor)
int extent_x = 21;
int extent_y = 37;
int extent_z = 41;
int size = extent_x * extent_y * extent_z;
int size = extent_x * extent_x * extent_z;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was probably uninteded, as it is currently breaking the tests.

// Should default to distribution BLOCKED, NONE, NONE:
dash::BlockPattern<3> pat_default(extent_x, extent_y, extent_z);
EXPECT_EQ(dash::DistributionSpec<3>(), pat_default.distspec());
EXPECT_EQ(dash::Team::All(), pat_default.team());
EXPECT_EQ(dash::Team::All().size(), pat_default.num_units());
EXPECT_EQ(size, pat_default.capacity());

dash::SizeSpec<3> sspec(
extent_x, extent_x, extent_z);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the wrong y-extent here, too.

dash::DistributionSpec<3> ds_blocked_z(
dash::NONE, dash::NONE, dash::BLOCKED);
dash::BlockPattern<3, dash::COL_MAJOR> pat_ds(
Expand All @@ -44,6 +46,23 @@ TEST_F(BlockPatternTest, SimpleConstructor)
EXPECT_EQ(ds_blocked_z, pat_ds_t.distspec());
EXPECT_EQ(size, pat_ds_t.capacity());
EXPECT_EQ(dash::Team::All().size(), pat_ds_t.num_units());

// As above but mix size and distribution specification.
// Has to start with a size specifier.
dash::BlockPattern<3> pat_mixed(
extent_x, dash::NONE, extent_y, dash::NONE, extent_z,
dash::BLOCKED,
dash::Team::All());
EXPECT_EQ(ds_blocked_z, pat_ds_t.distspec());
EXPECT_EQ(size, pat_ds_t.capacity());
EXPECT_EQ(dash::Team::All().size(), pat_ds_t.num_units());

dash::BlockPattern<3> pat_spec(
sspec, ds_blocked_z,
dash::Team::All());
EXPECT_EQ(ds_blocked_z, pat_ds_t.distspec());
EXPECT_EQ(size, pat_ds_t.capacity());
EXPECT_EQ(dash::Team::All().size(), pat_ds_t.num_units());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrong patterns are checked in those EXPECTs.

}

TEST_F(BlockPatternTest, EqualityComparison)
Expand Down