[AArch64] Fix DAG selection for cmps for fp16 type

[weimingz]

Summary

When emitting comparison for fp16, in addition to promote the LHS and RHS to fp32, we need to change the VT as well.

[t.p.northover]

I think the real bug here is in emitComparison, which has exactly this code to promote the arguments but still produces an f16 result. Changing that to f32 ought to fix things (though the dataflow between FCMP and FCCMP is a bit dubious, it's playing rather loose with types given that NZCV is the only register actually modified).

test/CodeGen/AArch64/half.ll

87–88 ↗

(On Diff #56157)

There are more ways this could go wrong than simply not emitting an fcmp/fccmp. I think you should check for an fcvt producing an s register that then gets used in the comparisons.

[weimingz]

This fixes the crash of "llc: tip/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6340: void llvm::SelectionDAG::ReplaceAllUsesWith(llvm::SDNode *, llvm::SDNode *): Assertion `(!From->hasAnyUseOfValue(i) || From->getValueType(i) == To->getValueType(i)) && "Cannot use this version of ReplaceAllUsesWith!"' failed.

[t.p.northover]

Thanks Weiming. Looks good to me.

Tim.

[weimingz]

Thank you, Tim! And do we need to guard the promotion of fp16->fp32 with !HasFullFP16() check? If some subtarget has full fp16 support, the promotion is not needed, is it?

lib/Target/AArch64/AArch64ISelLowering.cpp

1412 ↗

(On Diff #56157)

Operands get from a chained SETCC

[mgrang]

llvm/trunk/test/CodeGen/AArch64/half.ll

86

It would be good to follow the convention of having a space between ; and CHECK (see other instances in this file):

llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue emitComparison(SDValue LHS, SDValue RHS, ISD::CondCode CC,
                               SDLoc dl, SelectionDAG &DAG) {
   EVT VT = LHS.getValueType();
 
   if (VT.isFloatingPoint()) {
     assert(VT != MVT::f128);
     if (VT == MVT::f16) {
       LHS = DAG.getNode(ISD::FP_EXTEND, dl, MVT::f32, LHS);
       RHS = DAG.getNode(ISD::FP_EXTEND, dl, MVT::f32, RHS);
+      VT = MVT::f32;
     }
     return DAG.getNode(AArch64ISD::FCMP, dl, VT, LHS, RHS);
   }
 
   // The CMP instruction is just an alias for SUBS, and representing it as
   // SUBS means that it's possible to get CSE with subtract operations.
   // A later phase can perform the optimization of setting the destination
   // register to WZR/XZR if it ends up being unused.

llvm/trunk/test/CodeGen/AArch64/half.ll

 define void @test_trunc64(double %in, half* %addr) {
 ; CHECK-LABEL: test_trunc64:
 ; CHECK: fcvt {{h[0-9]+}}, {{d[0-9]+}}
 
   %val16 = fptrunc double %in to half
   store half %val16, half* %addr
   ret void
 }
+
+define i16 @test_fccmp(i1 %a) {
+;CHECK-LABEL: test_fccmp:
+;CHECK: fcmp
+  %cmp0 = fcmp ogt half 0xH3333, undef
+  %cmp1 = fcmp ogt half 0xH2222, undef
+  %x = select i1 %cmp0, i16 0, i16 undef
+  %or = or i1 %cmp1, %cmp0
+  %y = select i1 %or, i16 4, i16 undef
+  %r = add i16 %x, %y
+  ret i16 %r
+}