[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

7073

we -> We

7074–7075

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

7074–7075

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

7074–7075

I meant:

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

[stuij]

llvm/lib/Target/ARM/ARMISelLowering.cpp

7074–7075

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

7074–7075

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,
   EVT VT = Op.getValueType();
   bool IsDouble = (VT == MVT::f64);
   ConstantFPSDNode *CFP = cast<ConstantFPSDNode>(Op);
   const APFloat &FPVal = CFP->getValueAPF();
 
   // Prevent floating-point constants from using literal loads
   // when execute-only is enabled.
   if (ST->genExecuteOnly()) {
+    // We shouldn't trigger this for v6m execute-only
+    assert((!ST->isThumb1Only() || ST->hasV8MBaselineOps()) &&
+           "Unexpected architecture");
+
     // 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/test/CodeGen/ARM/float-ops-xo.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
 ; RUN: llc < %s -mtriple=thumbv8.1m.main-none-eabihf -mattr=+mve,+execute-only | FileCheck %s
+; RUN: llc < %s -mtriple=thumbv6m -mattr=+execute-only | FileCheck %s --check-prefix=V6M
 
 declare void @ext_fn(float)
 
 define void @test1() {
 ; CHECK-LABEL: test1:
 ; CHECK:       @ %bb.0: @ %entry
 ; CHECK-NEXT:    .save {r7, lr}
 ; CHECK-NEXT:    push {r7, lr}
 ; CHECK-NEXT:    movs r0, #0
 ; CHECK-NEXT:    vmov s0, r0
 ; CHECK-NEXT:    bl ext_fn
 ; CHECK-NEXT:    pop {r7, pc}
+;
+; V6M-LABEL: test1:
+; V6M:       @ %bb.0: @ %entry
+; V6M-NEXT:    push {r7, lr}
+; V6M-NEXT:    movs r0, #0
+; V6M-NEXT:    bl ext_fn
+; V6M-NEXT:    pop {r7, pc}
 entry:
   call void @ext_fn(float 0.000000e+00)
   ret void
 }
 
 define float @test2() {
 ; CHECK-LABEL: test2:
 ; CHECK:       @ %bb.0: @ %entry
 ; CHECK-NEXT:    movw r0, #63911
 ; CHECK-NEXT:    movt r0, #16673
 ; CHECK-NEXT:    vmov s0, r0
 ; CHECK-NEXT:    bx lr
+;
+; V6M-LABEL: test2:
+; V6M:       @ %bb.0: @ %entry
+; V6M-NEXT:    movs r0, #65
+; V6M-NEXT:    lsls r0, r0, #8
+; V6M-NEXT:    adds r0, #33
+; V6M-NEXT:    lsls r0, r0, #8
+; V6M-NEXT:    adds r0, #249
+; V6M-NEXT:    lsls r0, r0, #8
+; V6M-NEXT:    adds r0, #167
+; V6M-NEXT:    bx lr
 entry:
   ret float 0x40243F34E0000000
 }