-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[InstCombine] Incorrect propagation of nsz from fneg to fdiv #48998
Comments
ninf can't be propagated either. |
I think we can intersect 'nsz' and 'ninf' on this transform for the 2 instructions and be safe. Note that there's a difference in the LangRef about the consequences of 'nsz' vs. 'ninf': We made 'ninf' produce poison, but 'nsz' remained "insignificant", so that shows up in Alive2: That was part of: And came up as source of trouble here: |
Also note: 'nnan' is defined similarly to 'ninf', but it seems ok to propagate that flag only from "fneg". That's because we don't track the sign of a NaN value in default LLVM FP mode, and there's no way to create NaN with "fdiv Constant Number, X" that isn't also NaN with "fdiv -Negative Constant Number, X". Ie, either X has to be NaN or the constant is Inf/0.0 with same value X (0.0/0.0 --> NaN, Inf/Inf --> NaN). |
I pushed the minimal fix here: We should add more FMF tests and confirm that related transforms are behaving as expected. |
How hard do you think it would be to teach ChangedIRComparer to filter on floating point operators, and do you think that would be useful in finding other potential problems like this? |
I hadn't seen that before. Another possibility: |
The only thing my build bot does is to run LLVM's test suite with Alive2. So the FMF bugs I've posted were all exposed by the unit tests. But it makes sense to fuzz: if you feel we need to gain confidence on the FMF code, generating all 2/3 line instruction variations with each of the flags and then piping that through Alive2 should do the trick. CC John & Florian as they know more about fuzzing. John wrote a program a while back to generate all IR functions of a given size. Not sure it supported floats, though. |
I never tried enumeration of FP codes, but it's a good idea. I have a student who's currently working on a little mutation engine for the unit tests, I'll make sure it supports toggling these flags! |
IIUC, Alive2 supports the special-FP-value flags: nsz, ninf, nnan.
That could be fun/scary. :) |
Extended Description
This is from Transforms/InstCombine/fneg.ll, reduced from fast to just nsz.
The issue is that the original fdiv has no nsz but the optimized one has. In this case, nsz can be propagated, but not added (even if fneg is nsz).
define float @fdiv_op0_constant_fneg_fmf(float %x) {
%d = fdiv float 42.000000, %x
%r = fneg nsz float %d
ret float %r
}
=>
define float @fdiv_op0_constant_fneg_fmf(float %x) {
%r = fdiv nsz float -42.000000, %x
ret float %r
}
Transformation doesn't verify!
ERROR: Value mismatch
Example:
float %x = #x80000000 (-0.0)
Source:
float %d = #xff800000 (-oo)
float %r = #x7f800000 (+oo)
Target:
float %r = #xff800000 (-oo)
Source value: #x7f800000 (+oo)
Target value: #xff800000 (-oo)
The text was updated successfully, but these errors were encountered: