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

Fix undefined aliasing issue when compiling with clang-cl #1786

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bgottamd
Copy link

@bgottamd bgottamd commented Apr 4, 2024

Fixes the inner ValueT alias throw an undefined error when compiling with clang/clang-cl on Windows by converting the convertValue into a template function.

Copy link

linux-foundation-easycla bot commented Apr 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@bgottamd bgottamd marked this pull request as ready for review April 4, 2024 23:20
@bgottamd bgottamd requested a review from kmuseth as a code owner April 4, 2024 23:20
@bgottamd
Copy link
Author

Adding a quick bump. This small code change is essential so that Blender can support both clang - merged PR here - and openvdb. Without it, blender fails to build due to the cited issue.

@LazyDodo
Copy link

LazyDodo commented Jun 6, 2024

any progress here? We'd like to get this working out of the box for blender, and while i have no issue including selected upstream landed patches for our dependencies, upstream unreviewed patches are a bit of a different story.

@jo-rn
Copy link

jo-rn commented Jul 5, 2024

I've hit the same clang bug before and reported it: llvm/llvm-project#64996. Using -fno-delayed-template-parsing, as was suggested there, makes OpenVDB compile successfully with clang-cl.

@bgottamd
Copy link
Author

Thanks @jo-rn - this does fix the issue with the header file compilation in blender. However, there is some performance degradation when this flag is used:

File args fastest slowest avg
classroom 3840x2160 256 samples header template fix 1209 1217 1213
-fno-delayed-template-parsing 1218 1228 1223

This equates to ~1% drop in performance just from this flag. Hoping this templating change or a modification of the alias can still be merged in.

@LazyDodo
Copy link

Odd result, classroom is a blender 2.79 scene, openvdb wasn't integrated till blender 2.83, so the odds of it having an openvdb volume has to be near the low end of things. Did you put the -fno-delayed-template-parsing flag globally on the whole blender project? or isolated it to just the few openvdb related files that needed this fix?

@bgottamd
Copy link
Author

bgottamd commented Jul 19, 2024

I isolated the flag to projects that complained about the header issue. This ended up being only extern_mantaflow and cycles_scene

@anthony-linaro
Copy link

For those keeping track of this issue affecting Blender, I have created a PR to work around the issue using the approach described in this thread: https://projects.blender.org/blender/blender/pulls/126236

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

Successfully merging this pull request may close these issues.

4 participants