Skip to content

Change default to JIT without fma #2076

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.

Sign up for GitHub

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

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

Change default to JIT without fma #2076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/liboslexec/llvm_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1529,9 +1529,15 @@ LLVM_Util::make_jit_execengine(std::string* err, TargetISA requestedISA,

llvm::TargetOptions options;
// Enables FMA's in IR generation.
// However cpu feature set may or may not support FMA's independently
options.AllowFPOpFusion = jit_fma() ? llvm::FPOpFusion::Fast
: llvm::FPOpFusion::Standard;
// However cpu feature set may or may not support FMA's independently.
// Note: This is confusingly named, LLVM's FPOpFution choices are Strict
// (no fma used), Standard (fma used for actual fma circumstances), and
// Fast (fused ops beyond just fma's, any time it thinks it will be
// faster). Since we call the option "jit_fma", we interpret that as
// switching between Strict and Standard. We will need some other control
// if we ever want full "Fast" mode.
options.AllowFPOpFusion = jit_fma() ? llvm::FPOpFusion::Standard
: llvm::FPOpFusion::Strict;
// Unfortunately enabling UnsafeFPMath allows reciprocals, which we don't want for divides
// To match results for existing unit tests we might need to disable UnsafeFPMath
// TODO: investigate if reciprocals can be disabled by other means.
Expand Down
Binary file added testsuite/render-microfacet/ref/out-macos-arm-strict-alt.exr
View file
Open in desktop
Copy link
Contributor

@ThiagoIze ThiagoIze Feb 16, 2026

Choose a reason for hiding this comment

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

I wonder, can we now perhaps remove some of these alt reference images after updating the main reference image? And if so, can we lower the fail thresholds back down? And if the answer is yes to all that, I wonder if the same could be said for other tests?

Copy link
Collaborator Author

@lgritz lgritz Feb 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Possibly. I don't claim that with this change, this test or any other no longer has any minor platform-to-platform differences. But I hope that it's less now, or at least less prone to needing new ones added in the future as minor changes to hardware or dependencies are rolled out to the GHA runner pool. (And even if not, this PR has merit by making the internal behavior of this option now match what it's documented to do).

For those tests with wider thresholds or multiple reference images, it's quite tedious to determine which ones can be eliminated or how much the threshold can be reduced. The task is basically trial and error of eliminating one ref image, pushing the change, and seeing if it breaks one of the 20-ish CI tests or not.

Binary file not shown.
Loading