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

HLSL++ structs do not support move-semantics #40

Closed
egorodet opened this issue Jan 7, 2021 · 10 comments
Closed

HLSL++ structs do not support move-semantics #40

egorodet opened this issue Jan 7, 2021 · 10 comments
Assignees

Comments

@egorodet
Copy link
Contributor

egorodet commented Jan 7, 2021

HLSL++ vector and matric structs have user-defined copy constructor which breaks "rule of zero", but do not define copy assignment, move constructor and move assignment operators which also means that these types also do not follow "rule of five" resulting in missing support of move semantics and lower performance when used in STL containers like std::vector.

It seems like HLSL++ types do not need to have user-defined copy constructor. Removing of user-defined copy-constructors will let the compiler generate correct implementations of noexcept copy/move constructors and noexcept assignment operators unlocking the effective memory management in modern C++.

@redorav redorav self-assigned this Jan 7, 2021
@redorav
Copy link
Owner

redorav commented Jan 7, 2021

Hi @egorodet I think in theory you're right, although in practice I'm seeing the compiler is allowing std::move to happen on a vector with no issues both using LLVM and MSVC. Have you found otherwise in practice?

I actually am going to agree nonetheless that it's simpler to just remove the copy constructors everywhere and let the compiler do its thing which its going to do correctly anyway, as you say the standard is clear about move constructors in the presence of a user-provided copy constructor.

@redorav
Copy link
Owner

redorav commented Jan 7, 2021

As an aside, I have found through investigating some inconsistencies in the empty constructor, where for some structures I zero-initialize but for some I don't. I'm going to fix them all as well as part of that commit.

@redorav
Copy link
Owner

redorav commented Jan 7, 2021

Closed via 0a78236

@redorav redorav closed this as completed Jan 7, 2021
@egorodet
Copy link
Contributor Author

egorodet commented Jan 7, 2021

According to C++ standard std::move is used to indicate that an object may be "moved from" and C++ compilers silently default moves to copy constructor when move constructors are not provided or not generated automatically for types. This is done for backward compatibility with an old C++ code - it will work correctly, but not so fast as with correctly implemented move-constructors allowing to reuse existing memory without additional allocations.

@egorodet
Copy link
Contributor Author

egorodet commented Jan 7, 2021

In the last commit I've noticed that floatN vector types data is still not initialized in default constructors.

@redorav
Copy link
Owner

redorav commented Jan 7, 2021

Yeah I understand what you're saying wrt std::move. The piece of code I was trying out was

std::vector<float4x4> myVector;
myVector.push_back(float4x4());

std::vector<float4x4> myVector2;
myVector2 = std::move(myVector);

Looking at myVector, the move had correctly succeeded as it was now invalid and all the content belonged to myVector2, so I think shows it was allowing the move even though by the letter it shouldn't have.

@egorodet
Copy link
Contributor Author

egorodet commented Jan 7, 2021

std::move does not strictly mean to move or fail when no move constructor available. It is just an indication for compiler that it can reuse memory when it is possible (i.e. move constructor is available for the type). When actual move is not possible the compiler falls back to copy, even when you written std::move and does not produce any compilation errors - this behavior is according to C++11 standard.

Your example does not desribe the case with missing move constructor because you're moving the std::vector and it has proper move constructor even when it's element type float4x4 does not.

@redorav
Copy link
Owner

redorav commented Jan 7, 2021

I think I need to read up on move semantics more as I think I don't quite understand all the different subtleties involved. In your original message you were saying

resulting in missing support of move semantics and lower performance when used in STL containers like std::vector.

so I thought that float4x4 not having a move constructor would somehow inhibit std::vector's move operation, which is why I was doing the test. I suppose now you're talking about moving the float4x4 itself, or creating a structure on top that attempts to move it.

In terms of whether it's a compiler failure or not, it would be easy to check the disassembly to see what the compiler is doing and determine whether move is happening or not. Regardless, I think the solution you proposed and has gone in already is the correct one.

@egorodet
Copy link
Contributor Author

egorodet commented Jan 7, 2021

std::vector is using move semantics of its elements internally to optimize many operations, for example you can read this article explaining the details: https://embeddeduse.com/2016/05/25/performance-gains-through-cpp11-move-semantics/

@egorodet
Copy link
Contributor Author

egorodet commented Jan 9, 2021

User-defined copy constructor still exists for float4x4 matrix type, but should be removed:

hlslpp_inline float4x4(const float4x4& m)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants