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

Add FEEvaluationImplHangingNodes::RunTypes::scalar #12999

Merged

Conversation

peterrum
Copy link
Member

alternative to #12781

Comment on lines 104 to 123
template <typename T>
struct UnivariateShapeDataTrait;

template <>
struct UnivariateShapeDataTrait<double>
{
using value_type = double;
};

template <>
struct UnivariateShapeDataTrait<float>
{
using value_type = float;
};

template <typename T, std::size_t width>
struct UnivariateShapeDataTrait<VectorizedArray<T, width>>
{
using value_type = T;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

@kronbichler Do we have somewhere a functionality like this?

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think so, but it would be useful to have in a central place (read: vectorization.h). And in general, I think you can simplify this by saying

template <typename T>
struct UnivariateShapeDataTrait
{
  using value_type = T;
};

and then only a specialization for VectorizedArray<T, width>.

@peterrum peterrum force-pushed the FEEvaluationImplHangingNodes_scalar_ branch from f9dd536 to 38b0b56 Compare November 27, 2021 12:02
unsigned int d,
bool transpose,
bool skip_borders>
static inline DEAL_II_ALWAYS_INLINE void
Copy link
Member

Choose a reason for hiding this comment

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

How big is the effect of inlining this function? I am hesitant to inline this in debug mode, but it might make sense in release mode. Can you wrap this into an ifndef DEBUG?

{{0, points - 1, 0, points - 1}}}}}}}};

const std::
array<std::array<std::array<std::array<unsigned int, 3>, 2>, 2>, 3>
Copy link
Member

Choose a reason for hiding this comment

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

Can you use ndarray here and in a few other places?

Comment on lines 1694 to 1921
return (fe_degree == -1 || fe_degree > 2) ?
FEEvaluationImplHangingNodesRunnerTypes::vectorized :
FEEvaluationImplHangingNodesRunnerTypes::scalar;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we also run the vectorized case only if we have Number::size() > 2, because the vectorized code path executes two sides of an interpolation, so with 2 SIMD lanes the scalar code does strictly less operations.

@kronbichler
Copy link
Member

Can you rebase this to master to see if the code now runs?

@kronbichler
Copy link
Member

/rebuild

enum class VectorizationTypes
{
/**
* TODO
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand these?

}

template <unsigned int direction>
static inline DEAL_II_ALWAYS_INLINE void
Copy link
Member

Choose a reason for hiding this comment

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

Also here, I would not inline in debug mode.

}

template <bool do_x, bool do_y, bool do_z>
inline DEAL_II_ALWAYS_INLINE void
Copy link
Member

Choose a reason for hiding this comment

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

Same here

enum class HelperType
{
/**
* TODO
Copy link
Member

Choose a reason for hiding this comment

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

Expand comment.

@peterrum peterrum force-pushed the FEEvaluationImplHangingNodes_scalar_ branch from 6d7847a to 5d3a0f4 Compare November 29, 2021 17:13
@peterrum peterrum force-pushed the FEEvaluationImplHangingNodes_scalar_ branch from 5d3a0f4 to 6b70043 Compare November 29, 2021 17:33
Comment on lines 484 to 486
#ifndef DEBUG
inline DEAL_II_ALWAYS_INLINE
#endif
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about simplifying the call to these #ifdefs by defining (for this file only)

#ifdef DEBUG
#define DEAL_II_ALWAYS_INLINE_RELEASE
#else
#define DEAL_II_ALWAYS_INLINE_RELEASE DEAL_II_ALWAYS_INLINE
#endif

and then use this flag, and at the end of the relevant section call

#undef DEAL_II_ALWAYS_INLINE_RELEASE

I think this gives us a good intermediate state to merge for now. We can then later analyze and see if we want to make additional changes before the release.

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

It looks much better now.

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

2 participants