Extend templation in GenVector to better support vectorised Vc types. #394

cjones051073 · 2017-03-06T07:58:21Z

This PR extends the type templation that was already present in some classes in the GenVector library to improve support for using vectorised (Vc) types. Specifically it :-

Extends the templation to the Plane3D, Transformation3D and Translation3D types.
Where necessary provides specialised methods for the vector types, when the original code was not generic enough to work in both scalar and vector scenarios. Typically this happens in the case of conditionals, where the differences required (booleans versus masks) are difficult to avoid.

This PR is not complete, in that there are still some classes in GenVector that still do not support Vc types, as they are still not templated, such as the Rotation like transformations. It would be nice to add this at some point, but the code associated to these is more extensive (3DConversions.cxx for instance) and that will require some work.

My changes pass the built in ROOT tests. In addition I have prepared a simple test case for the Vc types (attached) that I have used to check the scalar and vector types give equivalent results. Tested on OS X with the compilation command

clang++ -O3 -mavx2 -mfma root-config --cflags -I/Users/chris/Projects/Vc/install/include main.cpp root-config --libs -lGenVector /Users/chris/Projects/Vc/install/lib/libVc.a

main-cpp.txt

… on their scalar type

…ix support for Vc types

…ddTemplates

…ests. Start with a limited scope and just update Plane3D, Translation3D and Transformation3D

…ddTemplates

…ort vector Scalar types

… scalars

…ddTemplates

vgvassilev · 2017-03-10T15:09:40Z

math/genvector/inc/Math/GenVector/Cartesian2D.h

   Scalar Mag2()  const { return fX*fX + fY*fY; }
-   Scalar R()     const { return std::sqrt( Mag2());}
-   Scalar Phi()   const { return (fX==0 && fY==0) ? 0.0 : atan2(fY,fX);}
+   Scalar R()     const { using namespace std; return sqrt( Mag2()); }


Hm, what is the advantage of the using namespace std; pattern, instead of using return std::sqrt...?

It is required to support Vc types. For Vc types you need to use the versions of sqrt etc. shipped as part of that library. Using std::sqrt does not allow this. The namespace trick means you use the std:: versions when appropriate, but also allows other implementations when not.

Huh, thanks for explaining! Neat and subtle at the same time. Btw, there is an inherited extra space after the open paren.

Btw, it's @lmoneta's call but I'd prefer to have a short comment saying what the pattern does, or maybe a #define R__enable_vc_types using namespace std; This would make this easier to read by an unarmed eye.

I dislike preprocessor directives unless absolutely required, and I don't think here adding one really helps. To my eye the pattern as is is clear, but as you say its up to @lmoneta . I would be OK with adding some sort of comment somewhere ?

I think a comment would be as clear and more concise than a ifdef

Hi,

I don't understand this. Are you sure it is needed for Vc ? In the past I have seen Vc replacing automatically its vectorised function implementations in the std namespace

I am sure I ran into some issue that lead me to doing this... Also note the templation is in principle there for other types than Vc. This change in principle supports types that do not extend the math functions under std::, as well as those that do.

vgvassilev · 2017-03-10T15:11:26Z

math/genvector/inc/Math/GenVector/DisplacementVector3D.h

-         *begin++ = a;
-         *begin++ = b;
-         *begin = c;
+        Scalar a,b,c = Scalar(0);


Wouldn't that leave a and b uninitialized?

Quite right.... Will fix.

Fix pushed.

…ddTemplates

cjones051073 · 2017-03-14T18:06:23Z

enable_if implementations updated.

vgvassilev · 2017-03-14T18:49:49Z

math/genvector/inc/Math/GenVector/DisplacementVector3D.h

      */
-      template <typename SCALAR = Scalar>
-      typename std::enable_if<std::is_arithmetic<SCALAR>::value, DisplacementVector3D>::type Unit() const
+      template <typename SCALAR = Scalar, typename std::enable_if<std::is_arithmetic<SCALAR>::value>::type * = nullptr>


Could you transform this:
template <typename SCALAR = Scalar, typename std::enable_if<std::is_arithmetic<SCALAR>::value>::type * = nullptr>

into

template <typename SCALAR = Scalar, typename = std::enable_if<std::is_arithmetic<SCALAR>::value>::type>

(Forget my last comment... having a bad comment day).

You are missing one typename, but yes, I can make that change..

Actually, it fails to compile when there are two methods. i.e.

/** return unit vector parallel to this (scalar) */ template <typename SCALAR = Scalar, typename = typename std::enable_if<std::is_arithmetic<SCALAR>::value>::type> DisplacementVector3D Unit() const { const auto tot = R(); return tot == 0 ? *this : DisplacementVector3D(*this) / tot; } /** return unit vector parallel to this (vector) */ template <typename SCALAR = Scalar, typename = typename std::enable_if<!std::is_arithmetic<SCALAR>::value>::type> DisplacementVector3D Unit() const { SCALAR tot = R(); tot(tot == SCALAR(0)) = SCALAR(1); return DisplacementVector3D(*this) / tot; }

gives

In file included from /Users/chris/Projects/ROOT/source/math/genvector/src/Boost.cxx:16: In file included from /Users/chris/Projects/ROOT/build/include/Math/GenVector/Boost.h:20: In file included from /Users/chris/Projects/ROOT/build/include/Math/GenVector/LorentzVector.h:23: /Users/chris/Projects/ROOT/build/include/Math/GenVector/DisplacementVector3D.h:351:28: error: class member cannot be redeclared DisplacementVector3D Unit() const ^ /Users/chris/Projects/ROOT/build/include/Math/GenVector/DisplacementVector3D.h:341:28: note: previous declaration is here DisplacementVector3D Unit() const ^

I'll stick with what is currently there and works.

Ah, indeed.

… expect speed up based on the SIMD register size is observed (with 10% safety factor)

cjones051073 · 2017-03-14T20:12:34Z

I have extended the Vc GenVector test as requested to include a speed test. The test asserts that, for an equal number of 'photons' the processing time for the vectorised test should be a factor of Vc::double_v::Size faster than the scalar test (to within 10% to account for fluctuating machine conditions etc.). Output looks like

Scalar best time        = 0.00278807
Vectorised Vc best time = 0.00138211
Vectorised Vc SIMD size = 2
Vectorised Vc speedup   = 2.01725

So indeed for the default (SSE4) vectorisation level with double a factor of 2 is observed.

Chris

vgvassilev · 2017-03-14T20:17:57Z

@phsft-bot build!

Thanks for the updated test Chris!

cjones051073 · 2017-03-14T20:26:13Z

running the test a few times it seems 10% is too tight a margin. The fluctuations from just rerunning the test a few times can be quite sizeable. To avoid false positives I have extended the margin to 25%.

vgvassilev · 2017-03-14T21:35:23Z

Hm..., now I am confused, according to the test results, the vectorisation on slc6 and centos7 has a negative effect...

vgvassilev · 2017-03-14T21:35:36Z

@phsft-bot build!

cjones051073 · 2017-03-14T21:38:00Z

why do you say that ?

cjones051073 · 2017-03-14T21:38:59Z

I know for a fact the results are as expected on SLC6. The results in the talk I posted where run on this platform.

vgvassilev · 2017-03-14T21:41:25Z

I am puzzled why this happens, do you have an idea (the invariant is the gcc49).

You say results on SLC6 are expected: you mean the ones we observed in the jenkins build?

cjones051073 · 2017-03-14T21:43:56Z

which jenkins build ?

Note that to get good SIMD results requires the test to run on a machine that properly supports SIMD instructions. As I mention in my talk if the machine you use is a VM, the results could be way off.

cjones051073 · 2017-03-14T21:45:03Z

The only Vc results I really trust are those run on a 'real' CPU, not a VM.

vgvassilev · 2017-03-14T21:49:15Z

Ok, then we should not assert failure from in test case. Let's just print out the information for reference.

cjones051073 · 2017-03-14T21:49:33Z

What sort of a machine do the tests

https://phsft-jenkins.cern.ch/job/root-pullrequests/127/BUILDTYPE=Debug,COMPILER=gcc49,LABEL=slc6/testReport/junit/projectroot/test/test_GenVector_Vc/

run on ?

My bet is this is not a real CPU that properly supports SIMD instructions (SSE4 in this case) or a VM machine where the timings cannot be trusted.

Sorry, but if you want a timing test for the Vc types you have to run this on a machine where the SIMD types are properly supported, and looking at the results above I would say this is currently not the case.

Chris

cjones051073 · 2017-03-14T21:51:05Z

I agree then. If you run the jenkins tests on VM then the timing tests cannot be trusted. I will disable the failure mode from them.

vgvassilev · 2017-03-14T21:52:14Z

Thanks! With that I think we are ready to land this nice piece of work given we get green light from jenkins.

…on a VM with poort SIMD support

cjones051073 · 2017-03-14T21:55:17Z

I've turned off the failure return value from the timing tests, as if they are run from VMs the results cannot really be trusted.

vgvassilev · 2017-03-14T21:57:58Z

@phsft-bot build!

vgvassilev · 2017-03-15T09:58:20Z

Finally merged! Thanks for the contribution. I am looking forward to reviewing more PR going in this direction!

cjones051073 · 2017-03-15T10:03:22Z

Many thanks for your help as well. I may well return to some of the other genvector classes at some point, the rotations for instance, as I think I will eventually have a use for them. That will be more work though... Hopefully next time things will be smoother as I now know a lot more about what is expected in a ROOT PR... ;)

vgvassilev

Some post-commit review comments. Could you address them please?

vgvassilev · 2017-03-15T11:08:17Z

test/testGenVectorVc.cxx

+
+template <typename POINT, typename VECTOR, typename FTYPE>
+inline
+   typename std::enable_if<std::is_arithmetic<typename POINT::Scalar>::value &&


@cjones051073 it seems that we omitted transforming this code here as a default template parameter.

Indeed, I over looked these instances of enable_if. will change them.

Done. I guess I should open a new PR for this small fix, as this one is closed.

Done. see PR #427.

vgvassilev · 2017-03-15T11:08:31Z

test/testGenVectorVc.cxx

+
+template <typename POINT, typename VECTOR, typename FTYPE>
+inline
+   typename std::enable_if<!std::is_arithmetic<typename POINT::Scalar>::value &&


vgvassilev · 2017-03-15T11:08:41Z

test/testGenVectorVc.cxx

+}
+
+template <typename POINT, typename VECTOR, typename PLANE>
+inline typename std::enable_if<std::is_arithmetic<typename POINT::Scalar>::value &&


vgvassilev · 2017-03-15T11:08:50Z

test/testGenVectorVc.cxx

+}
+
+template <typename POINT, typename VECTOR, typename PLANE, typename FTYPE = typename POINT::Scalar>
+inline typename std::enable_if<!std::is_arithmetic<typename POINT::Scalar>::value &&


…root-project#394)

cjones051073 added 26 commits February 25, 2017 15:40

Template the GenVector Plane3D, Transform3D and Translation3D classes…

76a8f9d

… on their scalar type

Use the 'using namespace std' trick with Cartesian3D and Plane3D to f…

7e9c2b7

…ix support for Vc types

Extend the 'using namespace std' trick to a few more classes

b15d62a

Change class to typename

2b2fc17

Explicitly cast some constants to the scalar type (needed for Vc types)

2e74f0d

Remove empty implementation files

c3175c6

Template the Boost classes

9bdfb8e

Remove empty implementation files for Boost classes

c97245c

Clean up some indentations

f12d368

Add some missing template types to the Boost classes

bbbbbd0

Remove empty Boost implementation file

9a0ee77

Small cleanups

371d50d

Clean up some initialisations

dcb0b30

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

29e7e30

…ddTemplates

Add Scalar/Vector Plane3D::Normalise() methods using SFINAE

14e73b4

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

27bbb57

…ddTemplates

Roll back some changes for now, as they are causing problems in the t…

196a00b

…ests. Start with a limited scope and just update Plane3D, Translation3D and Transformation3D

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

0ad5828

…ddTemplates

Add specific vectorised implementations of a few methods

1928c75

Extend ostream operator for Displacement and Position vectors to supp…

5ccdaea

…ort vector Scalar types

Fix the vector abs method (not fabs) and Vector Unit() for vectorised…

ef1aea9

… scalars

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

6cac16a

…ddTemplates

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

68ba28a

…ddTemplates

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

939b7cb

…ddTemplates

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

d9b810b

…ddTemplates

Add override qualifier

776ce1a

peremato assigned lmoneta Mar 6, 2017

vgvassilev reviewed Mar 10, 2017

View reviewed changes

Merge branch 'master' of github.com:root-mirror/root into GenVector-A…

fbe74d2

…ddTemplates

vgvassilev reviewed Mar 14, 2017

View reviewed changes

Extend the GenVector Vc test with a timing test that asserts that the…

b018a03

… expect speed up based on the SIMD register size is observed (with 10% safety factor)

add fabs on timing diff and extend Vc genvector expected diff to 25%

bb1e386

do not return a failure from the Vc speed test, as test could be run …

1786383

…on a VM with poort SIMD support

vgvassilev merged commit 8c30c8d into root-project:master Mar 15, 2017

vgvassilev reviewed Mar 15, 2017

View reviewed changes

This was referenced Mar 15, 2017

Fixes some final 'after merge' comments in PR #394 #427

Closed

Fix Transform3D * Plane3D multiplication operator #453

Merged

vgvassilev self-assigned this Mar 28, 2017

gganis pushed a commit to gganis/root that referenced this pull request Apr 3, 2017

Extend templation in GenVector to better support vectorised Vc types. (…

2b329f8

…root-project#394)

amadio pushed a commit to amadio/root that referenced this pull request May 23, 2018

Extend templation in GenVector to better support vectorised Vc types. (…

ddb6255

…root-project#394)

Extend templation in GenVector to better support vectorised Vc types. #394

Extend templation in GenVector to better support vectorised Vc types. #394

Uh oh!

Conversation

cjones051073 commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjones051073 Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjones051073 Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 14, 2017

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 14, 2017

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 14, 2017

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 14, 2017

Uh oh!

cjones051073 commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 14, 2017

Uh oh!

vgvassilev commented Mar 15, 2017

Uh oh!

cjones051073 commented Mar 15, 2017

Uh oh!

vgvassilev left a comment

cjones051073 Mar 10, 2017 •

edited

Loading

cjones051073 Mar 13, 2017 •

edited

Loading

vgvassilev commented Mar 14, 2017 •

edited

Loading