LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 38086 - InstCombiner::visitSelectInst incorrectly checks for nsz on an fcmp
Summary: InstCombiner::visitSelectInst incorrectly checks for nsz on an fcmp
Status: NEW
Alias: None
Product: libraries
Classification: Unclassified
Component: Scalar Optimizations (show other bugs)
Version: trunk
Hardware: PC Windows NT
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-06 14:39 PDT by Eli Friedman
Modified: 2021-11-01 06:20 PDT (History)
6 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eli Friedman 2018-07-06 14:39:17 PDT
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?
Comment 1 Sanjay Patel 2018-07-08 08:59:11 PDT
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.
Comment 2 Eli Friedman 2018-07-09 13:29:14 PDT
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.
Comment 3 Sanjay Patel 2018-07-10 05:57:05 PDT
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'.
Comment 4 Sanjay Patel 2018-07-10 05:58:54 PDT
>   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;
  }
Comment 5 Matt Arsenault 2018-09-05 09:52:28 PDT
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
Comment 6 Eli Friedman 2018-09-05 12:13:22 PDT
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.
Comment 7 Matt Arsenault 2018-09-05 12:29:55 PDT
(In reply to Eli Friedman from comment #6)
> 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.
Comment 8 Sanjay Patel 2018-10-30 14:54:20 PDT
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
Comment 9 Sanjay Patel 2018-11-02 12:02:47 PDT
(In reply to Eli Friedman from comment #0)
> 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.
Comment 10 Sanjay Patel 2018-11-02 12:18:32 PDT
(In reply to Matt Arsenault from comment #5)
> 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.
Comment 11 Sanjay Patel 2019-05-14 13:59:54 PDT
Proposal to allow FMF on FP select:
https://reviews.llvm.org/D61917
Comment 12 Sanjay Patel 2019-06-09 09:59:50 PDT
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.