[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

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

Operands get from a chained SETCC

[mgrang]

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

86 ↗

(On Diff #56847)

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

lib/Target/AArch64/AArch64ISelLowering.cpp

 /// depth to avoid stack overflow.
 static SDValue emitConjunctionDisjunctionTreeRec(SelectionDAG &DAG, SDValue Val,
     AArch64CC::CondCode &OutCC, bool Negate, SDValue CCOp,
     AArch64CC::CondCode Predicate) {
   // We're at a tree leaf, produce a conditional comparison operation.
   unsigned Opcode = Val->getOpcode();
   if (Opcode == ISD::SETCC) {
     SDValue LHS = Val->getOperand(0);
     SDValue RHS = Val->getOperand(1);
     ISD::CondCode CC = cast<CondCodeSDNode>(Val->getOperand(2))->get();
     bool isInteger = LHS.getValueType().isInteger();
     if (Negate)
       CC = getSetCCInverse(CC, isInteger);
     SDLoc DL(Val);
     // Determine OutCC and handle FP special case.
     if (isInteger) {
       OutCC = changeIntCCToAArch64CC(CC);
     } else {
       assert(LHS.getValueType().isFloatingPoint());
+      // Also handle f16, for which we need to do a f32 comparison.
+      if (LHS.getValueType() == MVT::f16) {
+        LHS = DAG.getNode(ISD::FP_EXTEND, DL, MVT::f32, LHS);
+        RHS = DAG.getNode(ISD::FP_EXTEND, DL, MVT::f32, RHS);
+      }
+
       AArch64CC::CondCode ExtraCC;
       changeFPCCToANDAArch64CC(CC, OutCC, ExtraCC);
       // Some floating point conditions can't be tested with a single condition
       // code. Construct an additional comparison in this case.
       if (ExtraCC != AArch64CC::AL) {
         SDValue ExtraCmp;
         if (!CCOp.getNode())
           ExtraCmp = emitComparison(LHS, RHS, CC, DL, DAG);

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
+;CHECK: fccmp
+  %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
+}