[FPEnv][ARM] Add lowering of STRICT_FSETCC and STRICT_FSETCCS

[john.brawn]

Summary

These can be lowered to code sequences using CMPFP and CMPFPE which then get selected to VCMP and VCMPE. The implementation isn't fully correct, as the chain operand isn't handled correctly, but resolving that looks like it would involve changes around FPSCR-handling instructions and how the FPSCR is modelled.

The fp-intrinsics test was already testing some of this but as the entire test was being XFAILed it wasn't noticed. Un-XFAIL the test and instead leave the cases where we aren't generating the right instruction sequences as FIXME.

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

3672

Why is it ok to turn a STRICT node into a non-strict node?

[john.brawn]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

3672

Well, it's what we're doing already in the case that a STRICT_FSETCCS has a condition code that LegalizeSetCCCondCode can make legal by swapping the operands - or at least I think so from reading the code, the situation where I'm seeing FSETCCS is from the expansion of STRICT_FP_TO_UINT where that doesn't happen so I don't know of a way to check if that is the case.

If converting to non-strict isn't right, then what should be done instead?

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

3672

That does look like a bug, but probably not tested.

The ARM target will need to replicate whatever it does for floating point ISD::SETCC to also handle ISD::STRICT_FSETCC/FSETCCS. That may require fixing the SELECT_CC FIXME below if ARM uses that. You may need new STRICT_ target specific nodes that have chain input and output. And new isel patterns to match them.

[john.brawn]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

3672

The ARM target has SETCC set to expand which means it gets converted to SELECT_CC. I would expect that a similar expansion for STRICT_FSETCC would require adding a STRICT_SELECT_CC node if we don't want to lose strictness.

I think probably it's easier for the ARM target to have lowering for STRICT_FSETCC and STRICT_FSETCCS, given that we have the VCMP and VCMPE instructions that correspond to the two, so I'll do that.

[john.brawn]

Change this so we instead add handling to the ARM target.

[dmgreen]

Is it possible for arm_cmpfpe have both a chain and a glue? I presume it's the cmp that would need the chain, not anything else that gets expanded?

llvm/test/CodeGen/ARM/fp-intrinsics.ll

73

This is the thing that only works when dp is enabled? What was the deal there?

Am I right in saying that they are not incorrect, just inefficient?

[samparker]

Is it possible for arm_cmpfpe have both a chain and a glue?

+1. The helper looks like it's barely used so hopefully this shouldn't be an invasion change?

[john.brawn]

In D73194#1849170, @dmgreen wrote:

Is it possible for arm_cmpfpe have both a chain and a glue? I presume it's the cmp that would need the chain, not anything else that gets expanded?

Doing this gets an assertion failure during instruction selection:

Assertion `NumMIOperands >= II.getNumOperands() && NumMIOperands <= II.getNumOperands() + II.getNumImplicitDefs() + NumImpUses && "#operands for dag node doesn't match .td file!"'

VCMPES has one implicit def (FPSCR_NZCV), so having either chain or glue the numbers balance out, but if you have both you have one more operand than what's expected and you get the assertion.

[samparker]

Even when adding SDNPHasChain to the node description?

[john.brawn]

In D73194#1851155, @samparker wrote:

Even when adding SDNPHasChain to the node description?

That was with SDNPHasChain. After some more investigation though it turns out the problem there was when I was generating the CMPFPE I was putting the chain at the end when it should be at the start, which means when ISel was happening it wasn't being moved to the end, which meant InstrEmitter wasn't noticing it and handling it correctly.

It turns out though that having chain and glue appears to have a different problem. After ISel we currently have:

  t22: i32,glue = VCMPS t3, t6, TargetConstant:i32<14>, Register:i32 $noreg
t23: i32,glue = FMSTAT TargetConstant:i32<14>, Register:i32 $noreg, t22:1

but adding a chain we have:

  t20: i32,ch,glue = VCMPES t3, t6, TargetConstant:i32<14>, Register:i32 $noreg, t0
t21: i32,glue = FMSTAT TargetConstant:i32<14>, Register:i32 $noreg, t20

FMSTAT is using t20 which is the i32 output of the VCMPES, but it should be using t20:2, the glue output of the VCMPES, and this causes problems later. I'm not sure what even is responsible for making this happen - it looks like it's either in the ARMGenDAGISel.inc table, or the code which interprets the table.

[dmgreen]

OK. It looks like that is at least not going to be simple to sort out immediately.

In the meantime, not crashing is certainly better than crashing, and I think the rest of this code looks good. It's just the strict ordering that we are missing. Please see if you can follow up with either adding a chain or not using glue.

LGTM otherwise.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

case ISD::STRICT_FSETCCS: {
     unsigned Offset = IsStrict ? 1 : 0;
     Tmp1 = Node->getOperand(0 + Offset);
     Tmp2 = Node->getOperand(1 + Offset);
     Tmp3 = Node->getOperand(2 + Offset);
     bool Legalized =
         LegalizeSetCCCondCode(Node->getValueType(0), Tmp1, Tmp2, Tmp3,
                               NeedInvert, dl, Chain, IsSignaling);
 
-    if (Legalized) {
+    // If the condition code can be legalized then we can convert to a SETCC
+    // using that condition code. If we have a strict SETCC the condition code
+    // may already be legal, in which case we similarly convert to a non-strict
+    // SETCC with the condition code we already have.
+    if (Legalized || IsStrict) {
       // If we expanded the SETCC by swapping LHS and RHS, or by inverting the
       // condition code, create a new SETCC node.
       if (Tmp3.getNode())
         Tmp1 = DAG.getNode(ISD::SETCC, dl, Node->getValueType(0),
                            Tmp1, Tmp2, Tmp3, Node->getFlags());
 
       // If we expanded the SETCC by inverting the condition code, then wrap
       // the existing SETCC in a NOT to restore the intended condition.
       if (NeedInvert)
         Tmp1 = DAG.getLogicalNOT(dl, Tmp1, Tmp1->getValueType(0));
 
       Results.push_back(Tmp1);
       if (IsStrict)
         Results.push_back(Chain);
 
       break;
     }
 
-    // FIXME: It seems Legalized is false iff CCCode is Legal. I don't
-    // understand if this code is useful for strict nodes.
-    assert(!IsStrict && "Don't know how to expand for strict nodes.");
-
     // Otherwise, SETCC for the given comparison type must be completely
     // illegal; expand it into a SELECT_CC.
     EVT VT = Node->getValueType(0);
     int TrueValue;
     switch (TLI.getBooleanContents(Tmp1.getValueType())) {
     case TargetLowering::ZeroOrOneBooleanContent:
     case TargetLowering::UndefinedBooleanContent:
       TrueValue = 1;

llvm/test/CodeGen/ARM/fp-intrinsics.ll

 ; RUN: llc -mtriple=armv8a-none-eabi %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-SP,CHECK-DP
 ; RUN: llc -mtriple=thumbv8m.main-none-eabi %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-NOSP,CHECK-NODP
 ; RUN: llc -mtriple=thumbv8m.main-none-eabi %s -o - -mattr=fp-armv8 | FileCheck %s --check-prefixes=CHECK,CHECK-SP,CHECK-DP
 ; RUN: llc -mtriple=thumbv8m.main-none-eabi %s -o - -mattr=fp-armv8sp | FileCheck %s --check-prefixes=CHECK,CHECK-SP,CHECK-NODP
 
 ; Check that constrained fp intrinsics are correctly lowered. In particular
 ; check that the valid combinations of single-precision and double-precision
 ; hardware being present or absent work as expected (i.e. we get an instruction
 ; when one is available, otherwise a libcall).
 
-; FIXME: Tests fails as various things in CodeGen and Target/ARM need fixing.
-; XFAIL: *
-
+; FIXME: We're not generating the right instructions for some of these
+; operations (see further FIXMEs down below).
 
 ; Single-precision intrinsics
 
 ; CHECK-LABEL: add_f32:
 ; CHECK-NOSP: bl __aeabi_fadd
 ; CHECK-SP: vadd.f32
 define float @add_f32(float %x, float %y) #0 {
   %val = call float @llvm.experimental.constrained.fadd.f32(float %x, float %y, metadata !"round.tonearest", metadata !"fpexcept.strict") #0

 ; CHECK-SP: vcvt.s32.f32
 define i32 @fptosi_f32(float %x) #0 {
   %val = call i32 @llvm.experimental.constrained.fptosi.f32(float %x, metadata !"fpexcept.strict") #0
   ret i32 %val
 }
 
 ; CHECK-LABEL: fptoui_f32:
 ; CHECK-NOSP: bl __aeabi_f2uiz
-; CHECK-SP: vcvt.u32.f32
+; FIXME-CHECK-SP: vcvt.u32.f32
 define i32 @fptoui_f32(float %x) #0 {
   %val = call i32 @llvm.experimental.constrained.fptoui.f32(float %x, metadata !"fpexcept.strict") #0
   ret i32 %val
 }
 
 ; CHECK-LABEL: sqrt_f32:
 ; CHECK-NOSP: bl sqrtf
 ; CHECK-SP: vsqrt.f32

 ; CHECK-DP: vcvt.s32.f64
 define i32 @fptosi_f64(double %x) #0 {
   %val = call i32 @llvm.experimental.constrained.fptosi.f64(double %x, metadata !"fpexcept.strict") #0
   ret i32 %val
 }
 
 ; CHECK-LABEL: fptoui_f64:
 ; CHECK-NODP: bl __aeabi_d2uiz
-; CHECK-DP: vcvt.u32.f64
+; FIXME-CHECK-DP: vcvt.u32.f64
 define i32 @fptoui_f64(double %x) #0 {
   %val = call i32 @llvm.experimental.constrained.fptoui.f64(double %x, metadata !"fpexcept.strict") #0
   ret i32 %val
 }
 
 ; CHECK-LABEL: sqrt_f64:
 ; CHECK-NODP: bl sqrt
 ; CHECK-DP: vsqrt.f64

 ; CHECK-DP: vcvt.f64.f32
 define double @fpext_f32(float %x) #0 {
   %val = call double @llvm.experimental.constrained.fpext.f64.f32(float %x, metadata !"fpexcept.strict") #0
   ret double %val
 }
 
 ; CHECK-LABEL: sitofp_f32_i32:
 ; CHECK-NOSP: bl __aeabi_i2f
-; CHECK-SP: vcvt.f32.s32
+; FIXME-CHECK-SP: vcvt.f32.s32
 define float @sitofp_f32_i32(i32 %x) #0 {
   %val = call float @llvm.experimental.constrained.sitofp.f32.i32(i32 %x, metadata !"round.tonearest", metadata !"fpexcept.strict") #0
   ret float %val
 }
 
 ; CHECK-LABEL: sitofp_f64_i32:
-; CHECK-NODP: bl __aeabi_i2d
-; CHECK-DP: vcvt.f64.s32
+; FIXME-CHECK-NODP: bl __aeabi_i2d
+; FIXME-CHECK-DP: vcvt.f64.s32
 define double @sitofp_f64_i32(i32 %x) #0 {
   %val = call double @llvm.experimental.constrained.sitofp.f64.i32(i32 %x, metadata !"round.tonearest", metadata !"fpexcept.strict") #0
   ret double %val
 }
 
 
 attributes #0 = { strictfp }