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
Should audit LLVM for incorrect uses of isFloatingPoint() #1342
Comments
pr970-1.patch This is the first of a small series of patches. It includes changes which |
pr970-2.patch |
pr970-3.patch First, in PatternMatch.h, the m_Neg matcher is incorrect for VP vectors. It Second, in GlobalOpt.cpp, the ShrinkGlobalToBoolean transformation excludes FP, |
pr970-4.patch In most cases, these local logic errors were harmless because callers operated This patch fixes the dead m_Neg matcher that patch 3 commented out. at lib/Transforms/Scalar/InstructionCombining.cpp:6788:
SI); This can pass vectors to createNeg and looks like it might miscompile code by at lib/Target/CBackend/CBackend.cpp:2129 This is now llvm/llvm-bugzilla-archive#1126 . at lib/Transforms/Scalar/InstructionCombining.cpp:325: Please review this call site in getComplexity. It is clearly reachable with FP
at lib/Transforms/Scalar/InstructionCombining.cpp:473: static inline Value *dyn_castNegVal(Value *V) { This could clearly be invoked with FP vector operands. With this patch, the
These would already have applied to scalar FP, so I have to assume that they're IEEE-approved. Just one more! |
pr970-5.patch
Which confusingly sent even floating-point vector operations through the This patch streamlines the logic. |
Oh, and one more thing... In LowerSelect.cpp: bool LowerSelect::runOnFunction(Function &F) { This pass is dead except for 'opt -lowerselect', so there's no client to say which (isFloatingPoint or |
Gordon, With a few minor cleanups, these patches have all been committed. Thanks for the great patches! Reid. |
Mine. |
Thanks, Reid. |
Extended Description
A lot of code uses X->getType()->isFloatingPoint() to disable transformations that are unsafe on FP math.
We should audit these and change them to call Ty->isFPOrFPVector() if they are also unsafe for vectors of
floats (which is probably most of them).
Of course, it would be far better to just have FADD/FMUL/etc instructions separate from ADD/MUL, etc,
but that will take time.
-Chris
The text was updated successfully, but these errors were encountered: