Fixes fast_math attribute plumbing to llvm - #7068
Open
corleypc wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The (@fast_math) supplied flags get incorrectly passed to LLVM in lb_run_fast_float_math_pass(), because fast_math_flags is tested against ordinals instead of the corresponding bit positions.
Fixing it unmasks another issue. Some of the flags can't be applied to some of the LLVM instructions in the switch here, because they alias other flags. Here is a minimal example (for LLVMUIToFP) of reassoc aliasing nneg and turning unsigned conversions to signed conversions (build with -o:speed AFTER fixing the flags in lb_run_fast_float_math_pass(), otherwise it is essentially passing a random combination of flags):
To fix this, the commit replaces the explicit switch with a call to LLVMCanValueUseFastMathFlags() which should exclude conversions (and likely include a few other benefiting cases not in the switch) and should be more future proof.
All this is guarded by an LLVM version check in the commit, cause LLVM 17 doesn't seem to have support for fast math flags.