[ARM] in LowerConstantFP, make sure we cover armv6-m execute-only

[stuij]

Summary

Currently in LowerConstantFP we only check for the presence of an FPU after we
handle execute-only (XO), which would give us incorrect codegen for XO targets
that don't have an FPU. By moving the hasVFP3Base() check up, we guard against
this.

It doesn't look like we're currently getting here when generating code for v6m, but
we're guarding against the future.

[dmgreen]

Do you have a testcase that you can add?

[stuij]

@dmgreen: see the description. It doesn't look like we get here currently when generating code.

[dmgreen]

I see. You are adding this for v6m, but v6m will never have legal FP types so will never get here?

We usually won't make changes like this if the code is already unreachable. Maybe turn them into a assert instead? In this case it looks fine though, presuming we don't want the combination of vfp2 and execute-only.

[stuij]

address review comments

[dmgreen]

Thanks. Sounds good to me.

llvm/lib/Target/ARM/ARMISelLowering.cpp

7077

we -> We

7078–7079

Perhaps just use an assert instead of a llvm_unreachable? It's common to add the message to asserts in LLVM code.

[stuij]

llvm/lib/Target/ARM/ARMISelLowering.cpp

7078–7079

According to the documentation, llvm_unreachable is preferable to an assert(0), which it effectively is in this case:
https://llvm.org/docs/CodingStandards.html#assert-liberally

There was a longish thread discussing this, and I still came away with unreachable being preferred.
https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587

[dmgreen]

llvm/lib/Target/ARM/ARMISelLowering.cpp

7078–7079

I meant:

assert((!ST->isThumb1Only() || ST->hasV8MBaselineOps()) && "Unexpected architecture");

[stuij]

llvm/lib/Target/ARM/ARMISelLowering.cpp

7078–7079

Yes, I get that, but reading the documentation:

"Use llvm_unreachable to mark a specific point in code that should never be reached. This is especially desirable for addressing warnings about unreachable branches, etc., but can be used whenever reaching a particular code path is unconditionally a bug (not originating from user input; see below) of some kind."

To me it reads that llvm_unreachable is preferable here.

[dmgreen]

llvm/lib/Target/ARM/ARMISelLowering.cpp

7078–7079

I don't think assert with a condition is discouraged, only assert(false) or assert(0). Those are better as llvm_unreachable

[stuij]

addressed review comments

[dmgreen]

Thanks. LGTM

llvm/lib/Target/ARM/ARMISelLowering.cpp

 
 SDValue ARMTargetLowering::LowerConstantFP(SDValue Op, SelectionDAG &DAG,
                                            const ARMSubtarget *ST) const {
   EVT VT = Op.getValueType();
   bool IsDouble = (VT == MVT::f64);
   ConstantFPSDNode *CFP = cast<ConstantFPSDNode>(Op);
   const APFloat &FPVal = CFP->getValueAPF();
 
+  // This also covers armv6-M Execute Only, as armv6-M doesn't have an FPU
+  if (!ST->hasVFP3Base())
+    return SDValue();
+
   // Prevent floating-point constants from using literal loads
   // when execute-only is enabled.
   if (ST->genExecuteOnly()) {
     // If we can represent the constant as an immediate, don't lower it
     if (isFPImmLegal(FPVal, VT))
       return Op;
     // Otherwise, construct as integer, and move to float register
     APInt INTVal = FPVal.bitcastToAPInt();
     SDLoc DL(CFP);
     switch (VT.getSimpleVT().SimpleTy) {
       default:
         llvm_unreachable("Unknown floating point type!");
         break;
       case MVT::f64: {
         SDValue Lo = DAG.getConstant(INTVal.trunc(32), DL, MVT::i32);
         SDValue Hi = DAG.getConstant(INTVal.lshr(32).trunc(32), DL, MVT::i32);
         return DAG.getNode(ARMISD::VMOVDRR, DL, MVT::f64, Lo, Hi);
       }
       case MVT::f32:
           return DAG.getNode(ARMISD::VMOVSR, DL, VT,
               DAG.getConstant(INTVal, DL, MVT::i32));
     }
   }
 
-  if (!ST->hasVFP3Base())
-    return SDValue();
-
   // Use the default (constant pool) lowering for double constants when we have
   // an SP-only FPU
   if (IsDouble && !Subtarget->hasFP64())
     return SDValue();
 
   // Try splatting with a VMOV.f32...
   int ImmVal = IsDouble ? ARM_AM::getFP64Imm(FPVal) : ARM_AM::getFP32Imm(FPVal);