Improve STRICT_FSETCC codegen in absence of no NaN

[thopre]

Summary

As for SETCC, use a less expensive condition code when generating
STRICT_FSETCC if the node is known not to have Nan.

[thopre]

Make FPMO const

[thopre]

Rebase

[thopre]

Ping?

[thopre]

Ping?

[thopre]

Ping?

[SjoerdMeijer]

llvm/test/CodeGen/AArch64/arm64-fcmp-no-nans-opt.ll

2

Since these tests run with -enable-no-nans-fp-math, I am wondering what condition we are testing:

if ((FPMO && FPMO->hasNoNaNs()) || TM.Options.NoNaNsFPMath)

I guess that is TM.Options.NoNaNsFPMath, so do we have/need checks for FPMO->hasNoNaNs()?

[thopre]

Don't check for instruction nonan flag since there's none for constrained fcmp

[SjoerdMeijer]

In D91972#2548455, @thopre wrote:

Don't check for instruction nonan flag since there's none for constrained fcmp

But that check seems useful for float instructions? In other words, can we keep the check for FPMO->hasNoNaNs(), but just add some new tests for that?

[thopre]

llvm/test/CodeGen/AArch64/arm64-fcmp-no-nans-opt.ll

2

Ah yes, this only makes sense for regular fcmp which can have fast math flag nnan. Constrained fcmp cannot.

[thopre]

In D91972#2548500, @SjoerdMeijer wrote:

In D91972#2548455, @thopre wrote:

Don't check for instruction nonan flag since there's none for constrained fcmp

But that check seems useful for float instructions? In other words, can we keep the check for FPMO->hasNoNaNs(), but just add some new tests for that?

Constrained FCmp instructions cannot be given fast-math flags in textual IR currently. I'm not sure if there's a way for the flag to be propagated from somewhere else. I'll try to dig a bit more. Bear with me

[thopre]

Rename testcase

[thopre]

In D91972#2548536, @thopre wrote:

In D91972#2548500, @SjoerdMeijer wrote:

In D91972#2548455, @thopre wrote:

Don't check for instruction nonan flag since there's none for constrained fcmp

But that check seems useful for float instructions? In other words, can we keep the check for FPMO->hasNoNaNs(), but just add some new tests for that?

Constrained FCmp instructions cannot be given fast-math flags in textual IR currently. I'm not sure if there's a way for the flag to be propagated from somewhere else. I'll try to dig a bit more. Bear with me

Constrained fcmp can have such flags since any FPMathOperation can have the NoNaN flag but I could not find a way to set it from looking at the code. There are no parameter attribute that would achieve it, no easy way to express it with llvm.assume and no obvious combine that would work. Since there's no way to test that code I think it's safer to leave it out. Worst case it's an optimisation that does not happen and we can patch it later.

[SjoerdMeijer]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7010

Nit: perhaps just make this an if-else.

llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll

3 ↗

(On Diff #322205)

Do we need to add tests for .f16 and .f64 just for completeness?

[thopre]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7010

I'm not sure what you mean, something like:

ISD::CondCode Condition = getFcmpCondCode(FPCmp->getPredicate());
if (TM.Options.NoNaNsFPMath)
  Opers.push_back(DAG.getCondCode(getFCmpCodeWithoutNaN(Condition)));
else
  Opers.push_back(DAG>getCondCode(Condition));

? I'd prefer to keep the current code since it mirrors what is done in visitFcmp and reflect better that even if a better condition code can be used (due to no NaN) we are still going to push it. YMMV of course.

llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll

3 ↗

(On Diff #322205)

I've only done the f64 because f16 does not appear to be supported on AArch64:

LLVM ERROR: Cannot select: 0x55d983a0fc58: f16,ch = AArch64ISD::STRICT_FCMP 0x55d9839a8938, 0x55d983a0fab8, 0x55d983a0fb88
  0x55d983a0fab8: f16,ch = CopyFromReg 0x55d9839a8938, Register:f16 %0
    0x55d983a0fa50: f16 = Register %0
  0x55d983a0fb88: f16,ch = CopyFromReg 0x55d9839a8938, Register:f16 %1
    0x55d983a0fb20: f16 = Register %1
In function: f16_constrained_fcmp_ueq

Happy to add it later if someone adds that support.

[thopre]

Add f64 tests

[SjoerdMeijer]

In D91972#2550797, @thopre wrote:

Add f64 tests

llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll

3 ↗

(On Diff #322205)

It is supported if you add -mattr=+fullfp16 to the command line.

[SjoerdMeijer]

llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll

3 ↗

(On Diff #322205)

And without it, it shouldn't crash, so that is a problem, but perhaps a different one.

[thopre]

Add f16 cases

[SjoerdMeijer]

LGTM

[thopre]

In D91972#2550830, @SjoerdMeijer wrote:

LGTM

Great, thanks Sjoerd!

[thopre]

In D91972#2550832, @thopre wrote:

In D91972#2550830, @SjoerdMeijer wrote:

LGTM

Great, thanks Sjoerd!

I had to revert it because the testcase was failing on the bot. I rebased the patch yesterday and it was passing locally with f16 and f64. Maybe a change between yesterday and today. I'll try to reproduce and reopen the diff if it needs changing.

[thopre]

Remove f16 tests which is not yet supported by AArch64 backend: https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp$2277

[thopre]

In D91972#2551343, @thopre wrote:

Remove f16 tests which is not yet supported by AArch64 backend: https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp$2277

@SjoerdMeijer Ok to commit without f16 tests given the above? I've tested in debug mode and thus assertion on this time and that passes without issue (fails in that mode with f16 cases)

[SjoerdMeijer]

In D91972#2551345, @thopre wrote:

In D91972#2551343, @thopre wrote:

Remove f16 tests which is not yet supported by AArch64 backend: https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp$2277

@SjoerdMeijer Ok to commit without f16 tests given the above? I've tested in debug mode and thus assertion on this time and that passes without issue (fails in that mode with f16 cases)

Ah, ok, sure.

Bonus points for following up to fix the f16 case then. ;-)

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

#include "llvm/IR/ConstrainedOps.def"
   default: break;
   case ISD::STRICT_FP_ROUND:
     Opers.push_back(
         DAG.getTargetConstant(0, sdl, TLI.getPointerTy(DAG.getDataLayout())));
     break;
   case ISD::STRICT_FSETCC:
   case ISD::STRICT_FSETCCS: {
     auto *FPCmp = dyn_cast<ConstrainedFPCmpIntrinsic>(&FPI);
-    Opers.push_back(DAG.getCondCode(getFCmpCondCode(FPCmp->getPredicate())));
+    ISD::CondCode Condition = getFCmpCondCode(FPCmp->getPredicate());
+    const auto *FPMO = dyn_cast<FPMathOperator>(&FPI);
+    if ((FPMO && FPMO->hasNoNaNs()) || TM.Options.NoNaNsFPMath)
+      Condition = getFCmpCodeWithoutNaN(Condition);
+    Opers.push_back(DAG.getCondCode(Condition));
     break;
   }
   }
 
   SDValue Result = DAG.getNode(Opcode, sdl, VTs, Opers, Flags);
   pushOutChain(Result, EB);
 
   SDValue FPResult = Result.getValue(0);

llvm/test/CodeGen/AArch64/arm64-fcmp-no-nans-opt.ll

+; RUN: llc < %s -mtriple=arm64-eabi -enable-no-nans-fp-math | FileCheck %s
+
+declare i1 @llvm.experimental.constrained.fcmp.f32(float, float, metadata, metadata)
+
+; CHECK-LABEL: @constrained_fcmp_ueq
+; CHECK: fcmp s0, s1
+; CHECK-NEXT: cset w0, eq
+; CHECK-NEXT: ret
+define i1 @constrained_fcmp_ueq(float %a, float %b) nounwind ssp {
+  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f32(float %a, float %b, metadata !"ueq", metadata !"fpexcept.strict")
+  ret i1 %cmp
+}
+
+; CHECK-LABEL: @constrained_fcmp_une
+; CHECK: fcmp s0, s1
+; CHECK-NEXT: cset w0, ne
+; CHECK-NEXT: ret
+define i1 @constrained_fcmp_une(float %a, float %b) nounwind ssp {
+  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f32(float %a, float %b, metadata !"une", metadata !"fpexcept.strict")
+  ret i1 %cmp
+}
+
+; CHECK-LABEL: @constrained_fcmp_ugt
+; CHECK: fcmp s0, s1
+; CHECK-NEXT: cset w0, gt
+; CHECK-NEXT: ret
+define i1 @constrained_fcmp_ugt(float %a, float %b) nounwind ssp {
+  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f32(float %a, float %b, metadata !"ugt", metadata !"fpexcept.strict")
+  ret i1 %cmp
+}
+
+; CHECK-LABEL: @constrained_fcmp_uge
+; CHECK: fcmp s0, s1
+; CHECK-NEXT: cset w0, ge
+; CHECK-NEXT: ret
+define i1 @constrained_fcmp_uge(float %a, float %b) nounwind ssp {
+  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f32(float %a, float %b, metadata !"uge", metadata !"fpexcept.strict")
+  ret i1 %cmp
+}
+
+; CHECK-LABEL: @constrained_fcmp_ult
+; CHECK: fcmp s0, s1
+; CHECK-NEXT: cset w0, lt
+; CHECK-NEXT: ret
+define i1 @constrained_fcmp_ult(float %a, float %b) nounwind ssp {
+  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f32(float %a, float %b, metadata !"ult", metadata !"fpexcept.strict")
+  ret i1 %cmp
+}
+
+; CHECK-LABEL: @constrained_fcmp_ule
+; CHECK: fcmp s0, s1
+; CHECK-NEXT: cset w0, le
+; CHECK-NEXT: ret
+define i1 @constrained_fcmp_ule(float %a, float %b) nounwind ssp {
+  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f32(float %a, float %b, metadata !"ule", metadata !"fpexcept.strict")
+  ret i1 %cmp
+}