MADEAPPS / newton-dynamics

Newton Dynamics is an integrated solution for real time simulation of physics environments.
http://www.newtondynamics.com
Other
936 stars 182 forks source link

Move the dBigVector pair into a named structure. #244

Closed iSLC closed 3 years ago

iSLC commented 3 years ago

Because dBigVector does not have a trivial constructor it must be moved into a named structure before it can be used like this.

Anonymous unions and structures are a compiler extension. Some compilers have more relaxed rules regarding this behavior (such as Clang or MSVC) because it can lead to undesired results. Whereas other compilers (such as GCC) are more strict and disallow code that can potentially lead to undefined behavior.

It may look a bit weird having it like this and a bit out of place but there's no other way to keep it as close to initial approach while still respecting compiler rules.

Should fix las issue in #241

JulioJerez commented 3 years ago

ok I see the pull request by although that will work, we nee to make the class complete define all teh constructors and force the to be inline. not doing that and copy constructors and the other will resolve to a memcopy. and we do not what that it is possible that CGG and Clang, and maybe even later VS are smart enough to no issuing the rep move but I knwo for a fact that VS 15 and 17 will issues ton of rep move and that will no be good. that class is use heavilly by teh joint solver, so it better be very effiecnt.

so this class is impmented like this

   struct Bisect
    {
        D_INLINE Bisect() = default;
        D_INLINE Bisect(const dBigVector & l, const dBigVector & h)
            :low(l), high(h)
        {
        }

        dBigVector low;
        dBigVector high;
    };

is implemented as

    struct ndData
    {
        D_INLINE ndData(const dFloat64 a)
            :m_low(a)
            ,m_high(a)
        {
        }

        D_INLINE ndData(const ndData& data)
            :m_low(data.m_low)
            ,m_high(data.m_high)
        {
        }

        D_INLINE ndData(const dBigVector& low, const dBigVector& high)
            :m_low(low)
            ,m_high(high)
        {
        }

        dBigVector m_low;
        dBigVector m_high;
    };
JulioJerez commented 3 years ago

please sync when you have time and tell me if is works now.

iSLC commented 3 years ago

Off-topic question. Did you remove the CMake code that adds the -mavx2 -mfma to compiler? Earlier when I tried to compile I got compiler errors that AVX and FMA instructions could not be used and when I looked the CMake code that added those compiler flags was not there anymore.

JulioJerez commented 3 years ago

Yes I realized that this was a mistake. This mad sence in newton 3.xx because the math library was not exposed to the Client app. But now that we expose it as an online function. It will fail if the Clint app does not compile with the same option. This works fine when I did it at first, but when I try in a pc that does not support avx, I start to crash because illegal instruction, so even dll will not work because of the inlines.

We do not really need it for most the code, only the avx2 solver need it and that code is all wrapped under cpp.

On Sat, Aug 7, 2021, 7:34 PM Sandu Liviu Catalin @.***> wrote:

Off-topic question. Did you remove the CMake code that adds the -mavx2 -mfma to compiler? Earlier when I tried to compile I got compiler errors that AVX and FMA instructions could not be used and when I looked the code that added those compiler flags was not there anymore.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MADEAPPS/newton-dynamics/pull/244#issuecomment-894733385, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6EPJHWFP5THI67RVAMHMDT3XULXANCNFSM5BXZJF4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

JulioJerez commented 3 years ago

thee one thing I do not know is if MigGw will required to specify -mavx2 -mfma for the avx2 solver. that's one of the tricks to get performance gain with tha solver. but It is use explicitly via intrinsic.

In CMake this is specifies just for file ndDynamicsUpdateAvx2.cpp, but onel for UNIX and windows, maybe this line belowe needs to includes the DEFINE for MINGW

if(MSVC AND NEWTON_ENABLE_AVX2_SOLVER)

set_source_files_properties(dNewton/ndDynamicsUpdateAvx2.cpp PROPERTIES COMPILE_FLAGS " /arch:AVX2 " ) endif()

if(UNIX AND NEWTON_ENABLE_AVX2_SOLVER)

set_source_files_properties(dNewton/ndDynamicsUpdateAvx2.cpp PROPERTIES COMPILE_FLAGS " -march=haswell " ) endif()

can you verify that, since I do no build MINGW? also I do not know what the compile option need to be. But not we can not set itglobally since I will fail even for systems that support it but the main App did compile with the same option. that was a mistake of mine.

On Sat, Aug 7, 2021 at 8:11 PM Julio Jerez @.***> wrote:

Yes I realized that this was a mistake. This mad sence in newton 3.xx because the math library was not exposed to the Client app. But now that we expose it as an online function. It will fail if the Clint app does not compile with the same option. This works fine when I did it at first, but when I try in a pc that does not support avx, I start to crash because illegal instruction, so even dll will not work because of the inlines.

We do not really need it for most the code, only the avx2 solver need it and that code is all wrapped under cpp.

On Sat, Aug 7, 2021, 7:34 PM Sandu Liviu Catalin @.***> wrote:

Off-topic question. Did you remove the CMake code that adds the -mavx2 -mfma to compiler? Earlier when I tried to compile I got compiler errors that AVX and FMA instructions could not be used and when I looked the code that added those compiler flags was not there anymore.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MADEAPPS/newton-dynamics/pull/244#issuecomment-894733385, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6EPJHWFP5THI67RVAMHMDT3XULXANCNFSM5BXZJF4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

JulioJerez commented 3 years ago

I added it like this now

if(MSVC AND NEWTON_ENABLE_AVX2_SOLVER)

set_source_files_properties(dNewton/ndDynamicsUpdateAvx2.cpp PROPERTIES COMPILE_FLAGS " /arch:AVX2 " ) endif()

if(MINGW AND NEWTON_ENABLE_AVX2_SOLVER)

set_source_files_properties(dNewton/ndDynamicsUpdateAvx2.cpp PROPERTIES COMPILE_FLAGS " -mavx2 -mfma " ) endif()

if(UNIX AND NEWTON_ENABLE_AVX2_SOLVER)

set_source_files_properties(dNewton/ndDynamicsUpdateAvx2.cpp PROPERTIES COMPILE_FLAGS " -march=haswell " ) endif()

please sync and tell me if it is right, since I have no way to know.

On Sat, Aug 7, 2021 at 8:25 PM Julio Jerez @.***> wrote:

thee one thing I do not know is if MigGw will required to specify -mavx2 -mfma for the avx2 solver. that's one of the tricks to get performance gain with tha solver. but It is use explicitly via intrinsic.

In CMake this is specifies just for file ndDynamicsUpdateAvx2.cpp, but onel for UNIX and windows, maybe this line belowe needs to includes the DEFINE for MINGW

if(MSVC AND NEWTON_ENABLE_AVX2_SOLVER)

set_source_files_properties(dNewton/ndDynamicsUpdateAvx2.cpp PROPERTIES COMPILE_FLAGS " /arch:AVX2 " ) endif()

if(UNIX AND NEWTON_ENABLE_AVX2_SOLVER)

set_source_files_properties(dNewton/ndDynamicsUpdateAvx2.cpp PROPERTIES COMPILE_FLAGS " -march=haswell " ) endif()

can you verify that, since I do no build MINGW? also I do not know what the compile option need to be. But not we can not set itglobally since I will fail even for systems that support it but the main App did compile with the same option. that was a mistake of mine.

On Sat, Aug 7, 2021 at 8:11 PM Julio Jerez @.***> wrote:

Yes I realized that this was a mistake. This mad sence in newton 3.xx because the math library was not exposed to the Client app. But now that we expose it as an online function. It will fail if the Clint app does not compile with the same option. This works fine when I did it at first, but when I try in a pc that does not support avx, I start to crash because illegal instruction, so even dll will not work because of the inlines.

We do not really need it for most the code, only the avx2 solver need it and that code is all wrapped under cpp.

On Sat, Aug 7, 2021, 7:34 PM Sandu Liviu Catalin < @.***> wrote:

Off-topic question. Did you remove the CMake code that adds the -mavx2 -mfma to compiler? Earlier when I tried to compile I got compiler errors that AVX and FMA instructions could not be used and when I looked the code that added those compiler flags was not there anymore.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MADEAPPS/newton-dynamics/pull/244#issuecomment-894733385, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6EPJHWFP5THI67RVAMHMDT3XULXANCNFSM5BXZJF4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .