-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
InstCombiner::visitSelectInst incorrectly checks for nsz on an fcmp #37434
Comments
LangRef says: So we're explicitly applying FMF to the input operands only (unlike FP binops). Does that definition enable some miscompile/mis-optimization? It's not clear to me how applying the flags to the select improves things. |
The IEEE754 definition of comparisons doesn't distinguish between +0 and -0. And the result is an i1, not a float. So I can't see what possible effect nsz could have on an fcmp, despite what LangRef says. If the select itself or the users of the select have nsz, we can use that to modify the result from -0 to +0. |
Ah, I see now. Yes, the fabs() fold is odd (that's the only 1 in IR?). There's inconsistency between where the LangRef/verifier allows FMF and the definition of FPMathOperator: static bool classof(const ConstantExpr *CE) { A select that returns an FP type is an FPMathOperator, right? I had a related problem in a recent ftrunc patch: |
Pasted the wrong chunk: static bool classof(const Instruction *I) { |
Nsz is relevant to matching min/max patterns from compare and select. The DAG combine for this checks nnan and nsz (though currently it’s only checking the global nsz) I also have some frustrations with the restrictions on where fast math flags apply. Inferring something from the inputs that doesn’t directly impact the result is useful given that not every single IR construct supports flags. For example the global nnan flag is still necessary if the input value is a function argument, so this needs to be inferred from the uses |
If the current LangRef rules somehow don't reflect the way you would like to handle floating-point operations, please propose a change. LangRef should correspond to what we actually implement. |
I was thinking the LangRef rules were the same for all the types of flags, but the wording is actually different for nnan and nsz. I think nnan applying to both the input and outputs is strange, particularly for an arbitrary function call where it would be quite reasonable to accept NaN inputs and never return a NaN. I think it would make sense if flags uniformly only impacted their results, but that introduces the non-instruction type of sources problem that I mentioned. I'm not sure what the solution is for that. |
Here's another case where we might have used FMF on an FP cast: define i1 @orderedLessZeroUIToFP_nnan(i32 %x) { |
Bug 39535 would appear to rule out checking the result of the select. Using a global check would go against the shift to IR/node-level FMF. I think we need to correct the LangRef and IR verifier, so we can put FMF on a FP select. |
I'm hoping to clean part of that up here: |
Proposal to allow FMF on FP select: |
Strictly speaking, this should resolve this bug: I'm proposing that as an intermediate step to allow fixing bug 42179 without making this problem worse. |
Extended Description
nsz doesn't affect the result of an fcmp.
Not sure what the right solution is here... should we put fast-math flags on selects? Check some function-global flag? Check if the result of the select is used by an nsz operation?
The text was updated successfully, but these errors were encountered: