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

[GPU] Fixes compile error on MSVC by disabling Whole Program Optimization #4197

Merged
merged 2 commits into from
Jul 7, 2020

Conversation

larshg
Copy link
Contributor

@larshg larshg commented Jun 16, 2020

Temporarily avoids errors like:
#3951
#1113

presumable also this one:
#1418 (I have run the example with this patch)

According to this, it can possible impose a penality of 3-4% performance in normal libraries.

An alternative approach might be to parse different compilerflags to NVCC, instead of all Host flags - however I haven't looked into that just yet.

@kunaltyagi kunaltyagi added module: cmake needs: code review Specify why not closed/merged yet labels Jun 16, 2020
@SergioRAgostinho
Copy link
Member

I'm not sure what to say here exactly. This is silently hiding an issue. Should we not provide instead an explicit work around that users need to be aware and activate, e.g. through a cache variable?

@larshg
Copy link
Contributor Author

larshg commented Jul 1, 2020

Can also print a warning when its get disabled due to people actively selecting to compile CUDA/GPU.

Since its difficult to know, that you have to set the WITH_GLOBALOPTIMIZATION_DISABLED, when compiling for CUDA/GPU on windows?

So one will probably end up compiling everything without the flag.
Experience a runtime error, spent time verifying own code.
Maybe search for similar errors, but many will probably just post an issue here, get told to set the flag and then recompile everything again.

If we just print a warning, the user does get a notice about Global optimization being disabled (can be hard to filter it between everything else), but at least things will work.

@SergioRAgostinho
Copy link
Member

Sound argument. From my side proceed with adding the message then.

@kunaltyagi kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jul 4, 2020
@larshg
Copy link
Contributor Author

larshg commented Jul 6, 2020

Yes, seems like a double unstable CI. But we can try it again :)

@SergioRAgostinho SergioRAgostinho removed the needs: more work Specify why not closed/merged yet label Jul 6, 2020
Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

Merge if CI green. 3rd time I'm running it.

  1. Failed with a (potentially) flaky test
  2. Failed because it ran out of storage space

@kunaltyagi kunaltyagi added changelog: fix Meta-information for changelog generation platform: windows labels Jul 7, 2020
@kunaltyagi kunaltyagi changed the title [GPU] Don't enable Whole Program Optimization if building GPU/CUDA. [GPU] Fixes compile error on MSVC by disabling Whole Program Optimization Jul 7, 2020
@kunaltyagi kunaltyagi merged commit 6aaade3 into PointCloudLibrary:master Jul 7, 2020
@larshg larshg deleted the DisableGLIfCudaOrGPU branch July 8, 2020 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Meta-information for changelog generation module: cmake platform: windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants