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

Support for LLVM 14 #1492

Merged
merged 3 commits into from Apr 28, 2022
Merged

Support for LLVM 14 #1492

merged 3 commits into from Apr 28, 2022

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Apr 19, 2022

API changes we had to take into account:

  • TargetRegistry.h location
  • No more DisableTailCalls field in PassManagerBuilder.

Signed-off-by: Larry Gritz lg@larrygritz.com

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 19, 2022

ONE test is failing, render-microfacet, which has some different specular sparkles than the reference image when compiled with clang/llvm 14.

It appears to be deterministic, just different than before. I assume this is just the result of some slightly different code being generated for certain math, maybe changed order of operations or differently selected instructions in llvm/clang 14, ending up with different round-off in a critical part where it actually makes different rays. And is especially noticeable in this 1 sample/pixel test image. But it's more than I would ideally like to see from just a compiler upgrade and no changes to the algorithms in testrender.

@fpsunflower You're the testrender expert. Do you think I should just check in a new reference image and call it a day? Or do you think it's worth my trying to actually figure out what changed?

@fpsunflower
Copy link
Contributor

Is there an easy way to extract the images to look at them? I suspect its not worth worrying about too much.

Curious if you noticed anything when testing this on the production renderer though.

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 20, 2022

On the tests, click "details", which will take you to the GH CI run. Then in the upper left, right above the listing of "jobs", you'll see where it says "Summary" -- click that. Then scroll down to the "artifacts" section at the bottom center -- there will be one for each failed test. That will download a zip file containing some of the contents of build/testsuite/... and in particular will contain the output of all failed tests (and probably some other cruft). So you'll have this osl-ubuntu-linux-bleeding-edge.zip file, so in your OSL tree (assuming you have built and done a "make test" first), then you can

cd build
unzip osl-ubuntu-linux-bleeding-edge.zip

answer "A" for overwrite all files. Then

cd testsuite/render-micofacet
iv out.exr test/out.exr

I haven't tested at work with the real renderer yet, because I haven't built and installed clang/llvm 14. I just took advantage of the ease of trying out in CI to see if I could patch up llvm14 support just by seeing what errors I got from the build, and there were only a couple errors that were fairly obvious to fix.

I'll try to build llvm14 when I get a chance, but it may be a couple days, it can be a bit of an ordeal of things don't go right out of the box.

API changes we had to take into account:
* TargetRegistry.h location
* No more DisableTailCalls field in PassManagerBuilder.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
…review comments

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Collaborator Author

lgritz commented Apr 26, 2022

Reporting back: When I build against LLVM 14 at work, I get the same change in speckles in the same test. Just that one.

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 26, 2022

There are several choices as I see it:

  1. Merge the PR as-is, live with the test failure until we either figure out and fix its cause or decide it's fine and merge an updated ref image.
  2. Merge the PR along with an updated ref image, who cares.
  3. Merge just the code fixes from this PR, but not the changes to ci.yml, until we either figure out the reason for the failure or decide it's not worth it and re-commit the ci.yml change along with the updated ref image. (Pros: unsticks anybody waiting for llvm14 support, passes tests in CI. Cons: we're not testing against llvm14 until we resolve it.)
  4. Don't merge until I fully work out why that test changed. (Pros: no failed CI tests. Cons: doesn't build for people who want to use LLVM 14.)

@AlexMWells
Copy link
Contributor

Any downside to option 3?

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 26, 2022

Option 3 commits the code changes, but not the CI changes. So the downside is merely that we won't be CI testing against LLVM 14 until we either amend the reference images or fully identify and fix whatever caused the change.

For example, will merging the free function patch break against LLVM 14 / clang 14? Nobody will know, because we won't be checking it with the CI.

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 26, 2022

The speckle changes may be totally harmless. Certainly in the past, compiler and llvm changes have occasionally changed the appearance of these images (which are low samples per pixel, in a naive test renderer that doesn't attempt to ameliorate it in any way, and involves sampling code that is probably very sensitive to any changes in the implementation of the math). There's nothing about the image that looks wrong per se, just different in the specular noise compared to before, in the usual way that rendered monte carlo images can tend to be slightly different on different platforms. So maybe we don't care unless somebody notices a real world problem.

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 26, 2022

I was hoping @fpsunflower would be able to view the image diff and give us a "yeah, that's what I expect from a slightly changed compile of testrender, don't worry about it" or a "hmmm, let's fully understand why it changed before accepting it."

@fpsunflower
Copy link
Contributor

Sorry I haven't been able to study the diff in detail.

Whats strange is that the shader in question doesn't actually do any math, so it seems like there's very limited room for the compiler to change something ....

LLVM is the compiler for the JIT, but. the rest of the code is still compiled with gcc with that CI setup, right ?

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 28, 2022

No, the whole codebase is compiled with clang 14!
It's not necessarily an issue with the shader, I was thinking more that it might be the math in testrender or the BSDFs.

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 28, 2022

I can separate it out, I guess, try to build with gcc but use llvm for the JIT, and see what happens to the test.

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 28, 2022

Sorry, I now think you were correct -- in the CI test that fails, we're compiling the main code with gcc11, but we're using clang14 to turn llvm-ops.cpp into bitcode, and using llvm14 to JIT.

Attaching a zip file to make it easy for you to look at the before and after images. Look primarily at the top edge of the leftmost sphere.

output.zip

@fpsunflower
Copy link
Contributor

Yeah that looks quite minimal ... I think we should just accept it and move on.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz lgritz merged commit 9cfca93 into AcademySoftwareFoundation:main Apr 28, 2022
@lgritz lgritz deleted the lg-llvm14 branch April 29, 2022 06:02
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.

None yet

3 participants