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.

Sign up for GitHub

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

Jump to bottom

Make vectors trivially relocatable. #337

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Make vectors trivially relocatable. #337

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
69b4559
Make vectors trivially relocatable.
sukeya Jul 18, 2023
56f83c3
Keep both ways of having default ctor or not.
sukeya Jul 21, 2023
abc7c5e
Fix using default ctor whatever value the flag is.
sukeya Jul 21, 2023
04950f4
Revert "Fix using default ctor whatever value the flag is."
sukeya Jul 22, 2023
9db4019
Fix using a build flag to use a macro.
sukeya Jul 22, 2023
c49dc99
[wip] add a test for ImathVec when using default.
sukeya Jul 22, 2023
5d4ed11
Remove Imath::Config from ImathTestVecUseDefaultConstructor.
sukeya Jul 22, 2023
63478fc
Revert "Remove Imath::Config from ImathTestVecUseDefaultConstructor."
sukeya Jul 22, 2023
536b430
Remove Imath::Config from ImathTestVecUseDefaultConstructor.
sukeya Jul 22, 2023
961ac87
Fix assertion errors and clean up.
sukeya Jul 24, 2023
03f055f
Update src/Imath/ImathVec.h
sukeya Mar 13, 2024
64674d1
Updata src/Imath/ImathVec.h
sukeya Mar 13, 2024
7ad5437
Update src/Imath/ImathVec.h
sukeya Mar 13, 2024
386faf1
Merge remote-tracking branch 'origin/main' into make_vecs_trivially_r…
sukeya Mar 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions config/ImathConfig.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,4 +174,9 @@
# endif
#endif

// Whether the copy and move constructors of Vec2, Vec3 and Vec4 are default or not.
#ifndef IMATH_VEC_BE_TRIVIALLY_RELOCATABLE
# define IMATH_VEC_BE_TRIVIALLY_RELOCATABLE 0
#endif
Comment on lines +177 to +180
Copy link

Choose a reason for hiding this comment

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

Author of P1144 here. :) I think it's great you're adopting the terminology — and orthogonally great that you're =default'ing all the things — but, looking at this PR, I think the focus on the word "relocatable" is actually a distraction. You should just say IMATH_VEC_RULE_OF_ZERO or something like that. After your changes, it is true that a Vec2<T> will be trivially relocatable if T is trivially relocatable; but it will also be trivially copyable if T is trivially copyable; trivially copy-assignable if T is trivially copy-assignable; and so on. There's nothing relocation-specific about this PR as far as I can tell. (Am I missing something?)

Followup: Why is this gated on a macro at all? The =default changes look like a strict improvement to me. If the non-defaulted implementations were in a .cpp file, so they were only compiled once, then there might be a reason to keep them; but nope, they're inline constexpr in the header! I think you should consider very hard whether there's any reason to ever set the macro to anything but 1. (...Is it that you need to have an opt-in way to avoid the ABI break that comes with Vec2 suddenly being passed in registers?)

Copy link

Choose a reason for hiding this comment

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

FYI @sukeya @cary-ilm, there is right now this week a PR to add fully-P1144-compliant __is_trivially_relocatable(T) to Clang (fixing a long-standing false-positive bug on Windows and also making it respect user-defined special members including operator= so as to avoid false positives on all platforms). If you're interested (professionally, financially, or just plain interested) in P1144 trivial relocatability, I'd really appreciate it if you'd take a look at llvm/llvm-project#84621 and weigh in with your thoughts. (Full disclosure: my impression is that the false-positives being fixed don't actually matter to @sukeya's use-case of thrust::async::copy. But I'd still really appreciate your taking a look, if you have interest in moving P1144 forward at all. It [and I mean that PR, specifically!] needs to see some customer demand in order to progress.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your advice! I commited your patches.
I'm busy now, but I'll see that PR when I have a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your advice! I committed your patches.
I'm busy now, but I'll see that PR when I have a time.

Copy link

Choose a reason for hiding this comment

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

@sukeya Thank you! I'd really appreciate it if you leave a 👍 reaction now, so you'll receive notifications that remind you to add a fuller comment when you have the time.


#endif // INCLUDED_IMATH_CONFIG_H
2 changes: 1 addition & 1 deletion src/Imath/ImathBoxAlgo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ clip (const T& p, const Box<T>& box) IMATH_NOEXCEPT
///

template <class T>
IMATH_HOSTDEVICE constexpr inline T
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 inline T
closestPointInBox (const T& p, const Box<T>& box) IMATH_NOEXCEPT
{
return clip (p, box);
Expand Down
45 changes: 42 additions & 3 deletions src/Imath/ImathVec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,24 @@ template <class T> class IMATH_EXPORT_TEMPLATE_TYPE Vec2
IMATH_HOSTDEVICE constexpr Vec2 (T a, T b) IMATH_NOEXCEPT;

/// Copy constructor
#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE != 0
IMATH_HOSTDEVICE constexpr Vec2 (const Vec2& v) IMATH_NOEXCEPT = default;
sukeya marked this conversation as resolved.
Show resolved Hide resolved
#else
IMATH_HOSTDEVICE constexpr Vec2 (const Vec2& v) IMATH_NOEXCEPT;
#endif

/// Construct from Vec2 of another base type
template <class S>
IMATH_HOSTDEVICE constexpr Vec2 (const Vec2<S>& v) IMATH_NOEXCEPT;

/// Assignment
/// Copy assignment
#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE != 0
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 Vec2&
operator= (const Vec2& v) IMATH_NOEXCEPT = default;
sukeya marked this conversation as resolved.
Show resolved Hide resolved
#else
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 const Vec2&
operator= (const Vec2& v) IMATH_NOEXCEPT;
#endif

/// Destructor
~Vec2 () IMATH_NOEXCEPT = default;
sukeya marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -369,7 +378,11 @@ template <class T> class IMATH_EXPORT_TEMPLATE_TYPE Vec3
IMATH_HOSTDEVICE constexpr Vec3 (T a, T b, T c) IMATH_NOEXCEPT;

/// Copy constructor
#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE != 0
IMATH_HOSTDEVICE constexpr Vec3 (const Vec3& v) IMATH_NOEXCEPT = default;
#else
IMATH_HOSTDEVICE constexpr Vec3 (const Vec3& v) IMATH_NOEXCEPT;
#endif

/// Construct from Vec3 of another base type
template <class S>
Expand All @@ -387,9 +400,14 @@ template <class T> class IMATH_EXPORT_TEMPLATE_TYPE Vec3
explicit IMATH_HOSTDEVICE IMATH_CONSTEXPR14
Vec3 (const Vec4<S>& v, InfException);

/// Assignment
/// Copy assignment
#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE != 0
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 Vec3&
operator= (const Vec3& v) IMATH_NOEXCEPT = default;
#else
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 const Vec3&
operator= (const Vec3& v) IMATH_NOEXCEPT;
#endif

/// Destructor
~Vec3 () IMATH_NOEXCEPT = default;
Expand Down Expand Up @@ -690,7 +708,11 @@ template <class T> class IMATH_EXPORT_TEMPLATE_TYPE Vec4
IMATH_HOSTDEVICE constexpr Vec4 (T a, T b, T c, T d) IMATH_NOEXCEPT;

/// Copy constructor
#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE != 0
IMATH_HOSTDEVICE constexpr Vec4 (const Vec4& v) IMATH_NOEXCEPT = default;
#else
IMATH_HOSTDEVICE constexpr Vec4 (const Vec4& v) IMATH_NOEXCEPT;
#endif

/// Construct from Vec4 of another base type
template <class S>
Expand All @@ -700,9 +722,14 @@ template <class T> class IMATH_EXPORT_TEMPLATE_TYPE Vec4
template <class S>
IMATH_HOSTDEVICE explicit constexpr Vec4 (const Vec3<S>& v) IMATH_NOEXCEPT;

/// Assignment
/// Copy assignment
#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE != 0
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 Vec4&
operator= (const Vec4& v) IMATH_NOEXCEPT = default;
#else
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 const Vec4&
operator= (const Vec4& v) IMATH_NOEXCEPT;
#endif

/// Destructor
~Vec4 () IMATH_NOEXCEPT = default;
Expand Down Expand Up @@ -1212,11 +1239,13 @@ IMATH_HOSTDEVICE constexpr inline Vec2<T>::Vec2 (T a, T b) IMATH_NOEXCEPT
y (b)
{}

#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE == 0
template <class T>
IMATH_HOSTDEVICE constexpr inline Vec2<T>::Vec2 (const Vec2& v) IMATH_NOEXCEPT
: x (v.x),
y (v.y)
{}
#endif

template <class T>
template <class S>
Expand All @@ -1225,6 +1254,7 @@ IMATH_HOSTDEVICE constexpr inline Vec2<T>::Vec2 (const Vec2<S>& v)
y (T (v.y))
{}

#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE == 0
template <class T>
IMATH_CONSTEXPR14 IMATH_HOSTDEVICE inline const Vec2<T>&
Vec2<T>::operator= (const Vec2& v) IMATH_NOEXCEPT
Expand All @@ -1233,6 +1263,7 @@ Vec2<T>::operator= (const Vec2& v) IMATH_NOEXCEPT
y = v.y;
return *this;
}
#endif

template <class T>
template <class S>
Expand Down Expand Up @@ -1618,12 +1649,14 @@ IMATH_HOSTDEVICE constexpr inline Vec3<T>::Vec3 (T a, T b, T c) IMATH_NOEXCEPT
z (c)
{}

#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE == 0
template <class T>
IMATH_HOSTDEVICE constexpr inline Vec3<T>::Vec3 (const Vec3& v) IMATH_NOEXCEPT
: x (v.x),
y (v.y),
z (v.z)
{}
#endif

template <class T>
template <class S>
Expand All @@ -1633,6 +1666,7 @@ IMATH_HOSTDEVICE constexpr inline Vec3<T>::Vec3 (const Vec3<S>& v)
z (T (v.z))
{}

#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE == 0
template <class T>
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 inline const Vec3<T>&
Vec3<T>::operator= (const Vec3& v) IMATH_NOEXCEPT
Expand All @@ -1642,6 +1676,7 @@ Vec3<T>::operator= (const Vec3& v) IMATH_NOEXCEPT
z = v.z;
return *this;
}
#endif

template <class T>
template <class S>
Expand Down Expand Up @@ -2093,13 +2128,15 @@ IMATH_HOSTDEVICE constexpr inline Vec4<T>::Vec4 (T a, T b, T c, T d)
w (d)
{}

#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE == 0
template <class T>
IMATH_HOSTDEVICE constexpr inline Vec4<T>::Vec4 (const Vec4& v) IMATH_NOEXCEPT
: x (v.x),
y (v.y),
z (v.z),
w (v.w)
{}
#endif

template <class T>
template <class S>
Expand All @@ -2110,6 +2147,7 @@ IMATH_HOSTDEVICE constexpr inline Vec4<T>::Vec4 (const Vec4<S>& v)
w (T (v.w))
{}

#if IMATH_VEC_BE_TRIVIALLY_RELOCATABLE == 0
template <class T>
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 inline const Vec4<T>&
Vec4<T>::operator= (const Vec4& v) IMATH_NOEXCEPT
Expand All @@ -2120,6 +2158,7 @@ Vec4<T>::operator= (const Vec4& v) IMATH_NOEXCEPT
w = v.w;
return *this;
}
#endif

template <class T>
template <class S>
Expand Down
7 changes: 7 additions & 0 deletions src/ImathTest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"
target_link_libraries(ImathHalfPerfTest Imath::Imath)
add_test(NAME Imath.half_perf_test COMMAND $<TARGET_FILE:ImathHalfPerfTest>)

add_executable(ImathVecTriviallyRelocatableTest vec_trivially_relocatable_main.cpp testVecTriviallyRelocatable.cpp)
set_target_properties(ImathVecTriviallyRelocatableTest PROPERTIES
RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"
)
target_link_libraries(ImathVecTriviallyRelocatableTest Imath::Imath)
add_test(NAME Imath.vec_trivially_relocatable_test COMMAND $<TARGET_FILE:ImathVecTriviallyRelocatableTest>)

function(DEFINE_IMATH_TESTS)
foreach(curtest IN LISTS ARGN)
add_test(NAME Imath.${curtest} COMMAND $<TARGET_FILE:ImathTest> ${curtest})
Expand Down
Loading
Loading