AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.05k stars 348 forks source link

LLVM opaque pointers support #1728

Closed brechtvl closed 10 months ago

brechtvl commented 10 months ago

Description

New LLVM versions require opaque pointers, that is you can no longer tell from a pointer value which data type it points to.

OptiX opaque pointers might not be working yet due to the conversion inBackendLLVM::llvm_store_value between int64 and char*, where we can no longer compare the pointer type. This function is called from many places and it's unclear to me when this happens.

According to a4e779c4 this code is only temporary, so this may resolve itself as the ustring refactoring gets completed.

Tests

No new tests added, entire testsuite tests this.

Checklist:

brechtvl commented 10 months ago

The pointcloud.batched and debug-uninit.batched CI failures I guess are a real issue to be solved. Not immediately clear why they would only happen on some configurations. Fixed.

AlexMWells commented 10 months ago
OptiX opaque pointers were not tested yet and left disabled. It is likely broken due to the sneaky conversion in
BackendLLVM::llvm_store_value between int64 and char*, but we can no longer compare the pointer type. This function is called from many places and it's unclear to me when this happens.

In an upcoming PR, StringPhase3, we remove all type mutability for strings, its either a ustring_pod (char *) or a ustringhash_pod (int64). So you might wait for that PR to flow in before this one as to be able to try and handle the situation above.

AlexMWells commented 10 months ago

@brechtvl the changes to batched_analysis look good. But you will want to rebase/resolve on main, as https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1717 has been merged in which had changes in batched_analysis.cpp.

There is also another large pending PR https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1732 which might resolve (or be closer to) some of the Optix issues, as all the stores should be explicit ustring (const char *) or ustringhash (uint64_t)

lgritz commented 10 months ago

I checked out this branch, rebased it myself and added minor fixups to address the merge conflict, and it all seems to work. In addition to passing CI (except for the MacOS 13 test which is broken for unrelated reasons), I confirmed that it works fine on my laptop with LLVM 15 (and thus with the opaque pointers enabled), as well as at work -- which used LLVM 14, so no opaque pointers, but at least that confirms that it doesn't break anything that worked before, including testing both batched and OptiX modes.

I wanted to update your tree for you to save you the work, and then approve the PR. But for some reason, GitHub won't let me push back to your branch, even though you have marked the PR as "Maintainers are allowed to edit this pull request."

I pushed it to my fork: https://github.com/lgritz/OpenShadingLanguage/tree/llvm-opaque-pointers

Brecht, if you want to grab it from there and fold it into this PR, I think we are ready to merge this, unless you think there is more work needed.

brechtvl commented 10 months ago

I made this PR match the branch in your fork now.

But, LLVM 15 + OptiX might be broken because of that remaining TODO. So I only enabled opaque pointers on LLVM 16+ now to be on the safe side.

This should be ready to merge now.

lgritz commented 10 months ago

I'm in favor of taking steps forward, even if not perfect. As long as we don't break anybody using LLVM 15 or older, anything that makes it work on 16+, even if only for some modes, is forward movement. We can fix what's still missing in subsequent patches.

lgritz commented 10 months ago

LGTM. Shall I merge?