InstCombiner::visitSelectInst incorrectly checks for nsz on an fcmp

[efriedma-quic]

Bugzilla Link	38086
Version	trunk
OS	Windows NT
CC	@fhahn,@ecnelises,@RKSimon,@arsenm,@rotateright

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?

[rotateright]

Strictly speaking, this should resolve this bug: https://reviews.llvm.org/rL362909 ...because we've removed the dependency on fcmp with nsz.

I'm proposing that as an intermediate step to allow fixing bug 42179 without making this problem worse.

[rotateright]

Proposal to allow FMF on FP select: https://reviews.llvm.org/D61917

[rotateright]

Nsz is relevant to matching min/max patterns from compare and select.

I'm hoping to clean part of that up here: https://reviews.llvm.org/D54001 ..and that's one step towards solving bug 39475.

[rotateright]

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?

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.

[rotateright]

Here's another case where we might have used FMF on an FP cast:

define i1 @​orderedLessZeroUIToFP_nnan(i32 %x) { ; CHECK-LABEL: @​orderedLessZeroUIToFP_nnan( ; CHECK-NEXT: ret i1 true ; %a = uitofp i32 %x to float %uge = fcmp nnan oge float %a, 0.000000e+00 ret i1 %uge }

https://reviews.llvm.org/D53874

[arsenm]

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.

[efriedma-quic]

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.

[arsenm]

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

[rotateright]

static bool classof(const ConstantExpr *CE) { return CE->getType()->isFPOrFPVectorTy() || CE->getOpcode() == Instruction::FCmp; }

Pasted the wrong chunk:

static bool classof(const Instruction *I) { return I->getType()->isFPOrFPVectorTy() || I->getOpcode() == Instruction::FCmp; }

[rotateright]

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) { return CE->getType()->isFPOrFPVectorTy() || CE->getOpcode() == Instruction::FCmp; }

A select that returns an FP type is an FPMathOperator, right?

I had a related problem in a recent ftrunc patch: https://reviews.llvm.org/D48085 ...we don't allow FMF on the int<->FP casts, but I wanted them to indicate 'nsz'.

[efriedma-quic]

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.

[rotateright]

LangRef says: "Any set of fast-math flags are legal on an fcmp instruction, but the only flags that have any effect on its semantics are those that allow assumptions to be made about the values of input arguments; namely nnan, ninf, and nsz."

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.