[ARM] llc -arm-execute-only with floating point runs into UNREACHABLE

[labrinea]

Summary

The DAG Combiner tries to generate literal pools for floating point constants when both -arm-execute-only and vfp instructions are enabled:
(a cond b) ? 1.0f : 2.0f -> load (tmp + ((a cond b) ? 0 : 4)

reproducer.ll:

define arm_aapcs_vfpcc float @lower_fpconst_select(float %f) {
  %cmp = fcmp nnan oeq float %f, 0.000000e+00
  %sel = select i1 %cmp, float 5.000000e+08, float 5.000000e+09
  ret float %sel
}

$ llc -mtriple=thumbv8m.main -arm-execute-only -mattr=fp-armv8 reproducer.ll
execute-only should not generate constant pools
UNREACHABLE executed at llvm/lib/Target/ARM/ARMISelLowering.cpp:7640!

[efriedma]

There are a bunch of places in SelectionDAG which generate constant pools for obscure constructs. Maybe we need a separate TargetLowering hook to suppress constant pools in general, rather than just suppressing this particular DAGCombine? (grep for getConstantPool in lib/CodeGen/SelectionDAG/ .)

Alternatively, how hard would it be to teach the ARM backend to put constant pools into a data section?

[labrinea]

Instead of suppressing this particular reproducer, teach the compiler to lower constant pools as global values (literals in the data section) when generating execute-only code.

[efriedma]

Most targets keep the constants in the MachineConstantPool, and emit them with AsmPrinter::EmitConstantPool. I would prefer to align with the way other targets handle constant pools, if it isn't too hard.

[christof]

The ARM code generator emits constant pools in a custom way. I believe that is to get them in-line with the code as a constant-pool in .text. But I think that Eli Friedman is right that for this use case it is possible to use the standard LLVM way of emitting them.

test/CodeGen/ARM/constantfp.ll

184 ↗

(On Diff #101550)

I assume 'tmp' here refers to the address of a constant pool that contains the 2 floating point constants. You could use something like 'ConstantPoolAddr' instead of 'tmp' to make that more explicit.

180 ↗

(On Diff #100991)

typo: independent

[labrinea]

When generating execute-only code invoke the generic AsmPrinter in order to place the literals in the data section, otherwise handle Constant Pools in EmitInstruction.

[labrinea]

Oops, I forgot to remove the llvm_unreachable() check from ARMISelLowering.

[christof]

This might be a bit more difficult then expected. The code generator assumes the constant pool is PC-rel addressable using ADR at the moment. This is not the case when the constant pool lives in a different section. I should have known, sorry.

test/CodeGen/ARM/constantfp.ll

205 ↗

(On Diff #101876)

I doubt this is correct. ADR constructs a PC relative address with 8-bit immediate, but the constant pool is in data which might not be in range. I would expect a movw/movt to load the constant pool's address.

[labrinea]

Fixed the way we materialise the address of a constant pool when generating execute-only code. Use movw/movt instead of pc-relative access.

[labrinea]

If find my original approach in diff1 less ad-hock than the latest one diff5. Lowering a Constant Pool as a Global Variable at the DAG level seems to me more legitimate than printing movw/movt for a LEApcrel machine instruction and then using the generic asm printer for the constant pool itself.

[christof]

This patch is definitely not the right approach. The LEA pseudo instruction should never be selected. I had a bit of a closer look and it seems that (ARMWrapper tconstpool) has various patterns that all match to PC relative pseudo instructions. They all should not happen in XO, so maybe this work is a bit more involved then expected. Also, we still have the ConstantIslandPass that is going to assume these constants pools are present in text and will split basic blocks for no good reason.

I have to agree with Alexandros that the earlier approach of custom lowering the constant pool is more in line with the way the ARM Target is structured. @efriedma: any different opinion?

lib/Target/ARM/ARMAsmPrinter.cpp

1250 ↗

(On Diff #102005)

These *LEApcrel are pseudo instructions that load a PC-relative address. They should never expand to MOVW/MOVT.

[efriedma]

Custom-lowering the constant pool to a global isn't that terrible, I guess... it's ugly, and we can't share the globals across basic blocks, but it should behave correctly, as far as I know.

lib/Target/ARM/ARMAsmPrinter.cpp

1250 ↗

(On Diff #102005)

Yes, we shouldn't be messing with ARMAsmPrinter::EmitInstruction.

[labrinea]

It's unfortunate that we cannot share the globals across basic blocks. However, lowering a constant pool as a global variable guarantees that execute-only plays well with all the position-independent addressing modes out of the box. In order to support those with execute-only we would need to add new instruction patterns and logic to handle them in various places, which might introduce new bugs. I've switched back to the previous approach and added a few more checks to verify the CodeGen with position-independent addressing modes. I've also removed few more unreachable checks that can now be reached with PIC. Christof are you happy with it?

[christof]

The execute-only and PIC features cannot be enabled together. You probably should leave that unreachable in lib/Target/ARM/ARMAsmPrinter.cpp in place. You might even add one under the case ARM::CONSTPOOL_ENTRY in that file for good measure.

Otherwise, I'm happy to approve the constpool to global constant lowering

test/CodeGen/ARM/constantfp.ll

226 ↗

(On Diff #102305)

• Sorry, but this is wrong. The point of execute-only is that there is no data access to any text section.

[labrinea]

Hi Christof,

I wasn't aware that PIC is not compatible with execute-only. Indeed a constant pool in text is being generated if I remove those checks. It contains the distance between the constant pool that is located in the data section and the address of the "current" instruction. I've kept those checks against constant pool generation and added another one in ARMAsmPrinter.cpp. I've also removed the PIC tests since they are now irrelevant.

Cheers,
Alexandros

[christof]

I was about to approve it but, I have 1 little nitpick that I almost missed. Sorry for the dribble feed. It's about the naming of the global.

lib/Target/ARM/ARMISelLowering.cpp

2690 ↗

(On Diff #102335)

I expect you meant to use CP (from ConstantPool) or something like that instead?

[labrinea]

Actually I was inspired by getPICLabel in ARMAsmPrinter.cpp but I can change it to 'CP' if it makes more sense.

[christof]

In D33773#780123, @labrinea wrote:

Actually I was inspired by getPICLabel in ARMAsmPrinter.cpp but I can change it to 'CP' if it makes more sense.

Yes please. Something that makes the connection with a constant pool clear would be helpful for people to make that connection later on.

[labrinea]

I've renamed the constant pool labels as suggested.

[christof]

LGTM

[labrinea]

I've upset a buildbot which runs the address sanitizer:
ERROR: AddressSanitizer:
stack-use-after-scope lib/Target/ARM/ARMISelLowering.cpp:2690
That Twine variable is used illegally. I am reopening the revision.

[labrinea]

The Address Sanitizer caught a stack-use-after-scope of a Twine variable. This is now fixed by passing the Twine directly as a function parameter.

[christof]

In D33773#783691, @labrinea wrote:

The Address Sanitizer caught a stack-use-after-scope of a Twine variable. This is now fixed by passing the Twine directly as a function parameter.

That sounds like a good fix to me. Please mention the svn revision number of the original commit you reapply here. The git revision number has not much meaning.

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

EmitToStreamer(*OutStreamer, MCInstBuilder(Opcode)
       .addImm(0)
       // Add predicate operands.
       .addImm(MI->getOperand(3).getImm())
       .addReg(MI->getOperand(4).getReg()));
 
     return;
   }
   case ARM::CONSTPOOL_ENTRY: {
+    if (Subtarget->genExecuteOnly())
+      llvm_unreachable("execute-only should not generate constant pools");
+
     /// CONSTPOOL_ENTRY - This instruction represents a floating constant pool
     /// in the function.  The first operand is the ID# for this instruction, the
     /// second is the index into the MachineConstantPool that this is, the third
     /// is the size in bytes of this constant pool entry.
     /// The required alignment is specified on the basic block holding this MI.
     unsigned LabelId = (unsigned)MI->getOperand(0).getImm();
     unsigned CPIdx   = (unsigned)MI->getOperand(1).getIndex();
 

llvm/trunk/lib/Target/ARM/ARMISelLowering.h

SDValue LowerMemOpCallTo(SDValue Chain, SDValue StackPtr, SDValue Arg,
                              const CCValAssign &VA,
                              ISD::ArgFlagsTy Flags) const;
     SDValue LowerEH_SJLJ_SETJMP(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerEH_SJLJ_LONGJMP(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerEH_SJLJ_SETUP_DISPATCH(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG,
                                     const ARMSubtarget *Subtarget) const;
     SDValue LowerBlockAddress(SDValue Op, SelectionDAG &DAG) const;
+    SDValue LowerConstantPool(SDValue Op, SelectionDAG &DAG) const;
+    SDValue LowerGlobalAddress(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalAddressDarwin(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalAddressELF(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalAddressWindows(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalTLSAddress(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerToTLSGeneralDynamicModel(GlobalAddressSDNode *GA,
                                             SelectionDAG &DAG) const;
     SDValue LowerToTLSExecModels(GlobalAddressSDNode *GA,
                                  SelectionDAG &DAG,

llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp

 }
 
 // ConstantPool, JumpTable, GlobalAddress, and ExternalSymbol are lowered as
 // their target counterpart wrapped in the ARMISD::Wrapper node. Suppose N is
 // one of the above mentioned nodes. It has to be wrapped because otherwise
 // Select(N) returns N. So the raw TargetGlobalAddress nodes, etc. can only
 // be used to form addressing mode. These wrapped nodes will be selected
 // into MOVi.
-static SDValue LowerConstantPool(SDValue Op, SelectionDAG &DAG) {
+SDValue ARMTargetLowering::LowerConstantPool(SDValue Op,
+                                             SelectionDAG &DAG) const {
   EVT PtrVT = Op.getValueType();
   // FIXME there is no actual debug info here
   SDLoc dl(Op);
   ConstantPoolSDNode *CP = cast<ConstantPoolSDNode>(Op);
   SDValue Res;
+
+  // When generating execute-only code Constant Pools must be promoted to the
+  // global data section. It's a bit ugly that we can't share them across basic
+  // blocks, but this way we guarantee that execute-only behaves correct with
+  // position-independent addressing modes.
+  if (Subtarget->genExecuteOnly()) {
+    auto AFI = DAG.getMachineFunction().getInfo<ARMFunctionInfo>();
+    auto T = const_cast<Type*>(CP->getType());
+    auto C = const_cast<Constant*>(CP->getConstVal());
+    auto M = const_cast<Module*>(DAG.getMachineFunction().
+                                 getFunction()->getParent());
+    auto L = Twine(DAG.getDataLayout().getPrivateGlobalPrefix()) + "CP" +
+             Twine(DAG.getMachineFunction().getFunctionNumber()) + "_" +
+             Twine(AFI->createPICLabelUId());
+    auto GV = new GlobalVariable(*M, T, /*isConstant=*/true,
+                                 GlobalVariable::InternalLinkage, C, L);
+    SDValue GA = DAG.getTargetGlobalAddress(dyn_cast<GlobalValue>(GV),
+                                            dl, PtrVT);
+    return LowerGlobalAddress(GA, DAG);
+  }
+
   if (CP->isMachineConstantPoolEntry())
     Res = DAG.getTargetConstantPool(CP->getMachineCPVal(), PtrVT,
                                     CP->getAlignment());
   else
     Res = DAG.getTargetConstantPool(CP->getConstVal(), PtrVT,
                                     CP->getAlignment());
   return DAG.getNode(ARMISD::Wrapper, dl, MVT::i32, Res);
 }

 
 static bool isReadOnly(const GlobalValue *GV) {
   if (const GlobalAlias *GA = dyn_cast<GlobalAlias>(GV))
     GV = GA->getBaseObject();
   return (isa<GlobalVariable>(GV) && cast<GlobalVariable>(GV)->isConstant()) ||
          isa<Function>(GV);
 }
 
+SDValue ARMTargetLowering::LowerGlobalAddress(SDValue Op,
+                                              SelectionDAG &DAG) const {
+  switch (Subtarget->getTargetTriple().getObjectFormat()) {
+  default: llvm_unreachable("unknown object format");
+  case Triple::COFF:
+    return LowerGlobalAddressWindows(Op, DAG);
+  case Triple::ELF:
+    return LowerGlobalAddressELF(Op, DAG);
+  case Triple::MachO:
+    return LowerGlobalAddressDarwin(Op, DAG);
+  }
+}
+
 SDValue ARMTargetLowering::LowerGlobalAddressELF(SDValue Op,
                                                  SelectionDAG &DAG) const {
   EVT PtrVT = getPointerTy(DAG.getDataLayout());
   SDLoc dl(Op);
   const GlobalValue *GV = cast<GlobalAddressSDNode>(Op)->getGlobal();
   const TargetMachine &TM = getTargetMachine();
   bool IsRO = isReadOnly(GV);
 
static SDValue LowerFPOWI(SDValue Op, const ARMSubtarget &Subtarget,
   // Return the chain (the DAG root) if it is a tail call
   return !CI.second.getNode() ? DAG.getRoot() : CI.first;
 }
 
 SDValue ARMTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
   switch (Op.getOpcode()) {
   default: llvm_unreachable("Don't know how to custom lower this!");
   case ISD::WRITE_REGISTER: return LowerWRITE_REGISTER(Op, DAG);
-  case ISD::ConstantPool:
-    if (Subtarget->genExecuteOnly())
-      llvm_unreachable("execute-only should not generate constant pools");
-    return LowerConstantPool(Op, DAG);
+  case ISD::ConstantPool: return LowerConstantPool(Op, DAG);
   case ISD::BlockAddress:  return LowerBlockAddress(Op, DAG);
-  case ISD::GlobalAddress:
-    switch (Subtarget->getTargetTriple().getObjectFormat()) {
-    default: llvm_unreachable("unknown object format");
-    case Triple::COFF:
-      return LowerGlobalAddressWindows(Op, DAG);
-    case Triple::ELF:
-      return LowerGlobalAddressELF(Op, DAG);
-    case Triple::MachO:
-      return LowerGlobalAddressDarwin(Op, DAG);
-    }
+  case ISD::GlobalAddress: return LowerGlobalAddress(Op, DAG);
   case ISD::GlobalTLSAddress: return LowerGlobalTLSAddress(Op, DAG);
   case ISD::SELECT:        return LowerSELECT(Op, DAG);
   case ISD::SELECT_CC:     return LowerSELECT_CC(Op, DAG);
   case ISD::BR_CC:         return LowerBR_CC(Op, DAG);
   case ISD::BR_JT:         return LowerBR_JT(Op, DAG);
   case ISD::VASTART:       return LowerVASTART(Op, DAG);
   case ISD::ATOMIC_FENCE:  return LowerATOMIC_FENCE(Op, DAG, Subtarget);
   case ISD::PREFETCH:      return LowerPREFETCH(Op, DAG, Subtarget);

llvm/trunk/test/CodeGen/ARM/constantfp.ll

 ; RUN: llc -mtriple=armv7 -mattr=+neon -mcpu=swift %s -o - | FileCheck %s
 ; RUN: llc -mtriple=armv7 -mattr=+neon -mcpu=cortex-a8 %s -o - | FileCheck --check-prefix=CHECK-NONEONFP %s
 ; RUN: llc -mtriple=armv7 -mattr=-neon -mcpu=cortex-a8 %s -o - | FileCheck --check-prefix=CHECK-NONEON %s
 
 ; RUN: llc -mtriple=thumbv7m -mcpu=cortex-m4 %s -o - \
 ; RUN: | FileCheck --check-prefix=CHECK-NO-XO %s
 
 ; RUN: llc -mtriple=thumbv7m -arm-execute-only -mcpu=cortex-m4 %s -o - \
 ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT --check-prefix=CHECK-XO-DOUBLE %s
 
 ; RUN: llc -mtriple=thumbv7meb -arm-execute-only -mcpu=cortex-m4 %s -o - \
 ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT --check-prefix=CHECK-XO-DOUBLE-BE %s
 
+; RUN: llc -mtriple=thumbv7m -arm-execute-only -mcpu=cortex-m4 -relocation-model=ropi %s -o - \
+; RUN: | FileCheck --check-prefix=CHECK-XO-ROPI %s
+
 ; RUN: llc -mtriple=thumbv8m.main -mattr=fp-armv8 %s -o - \
 ; RUN: | FileCheck --check-prefix=CHECK-NO-XO %s
 
 ; RUN: llc -mtriple=thumbv8m.main -arm-execute-only -mattr=fp-armv8 %s -o - \
 ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT --check-prefix=CHECK-XO-DOUBLE %s
 
 ; RUN: llc -mtriple=thumbv8m.maineb -arm-execute-only -mattr=fp-armv8 %s -o - \
 ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT --check-prefix=CHECK-XO-DOUBLE-BE %s
 
+; RUN: llc -mtriple=thumbv8m.main -arm-execute-only -mattr=fp-armv8 -relocation-model=ropi %s -o - \
+; RUN: | FileCheck --check-prefix=CHECK-XO-ROPI %s
 
 define arm_aapcs_vfpcc float @test_vmov_f32() {
 ; CHECK-LABEL: test_vmov_f32:
 ; CHECK: vmov.f32 d0, #1.0
 
 ; CHECK-NONEONFP: vmov.f32 s0, #1.0
   ret float 1.0
 }

 ; CHECK-XO-DOUBLE-BE: movw [[REG1:r[0-9]+]], #27263
 ; CHECK-XO-DOUBLE-BE: movw [[REG2:r[0-9]+]], #6291
 ; CHECK-XO-DOUBLE-BE: movt [[REG1]], #29884
 ; CHECK-XO-DOUBLE-BE: movt [[REG2]], #16340
 ; CHECK-XO-DOUBLE-BE: vmov {{d[0-9]+}}, [[REG2]], [[REG1]]
 ; CHECK-XO-DOUBLE-BE-NOT: vldr
   ret double 3.140000e-01
 }
+
+; This is a target independent optimization, performed by the
+; DAG Combiner, which promotes floating point literals into
+; constant pools:
+;
+; (a cond b) ? 1.0f : 2.0f -> load (ConstPoolAddr + ((a cond b) ? 0 : 4)
+;
+; We need to make sure that the constant pools are placed in
+; the data section when generating execute-only code:
+
+define arm_aapcs_vfpcc float @lower_fpconst_select(float %f) {
+
+; CHECK-NO-XO-LABEL: lower_fpconst_select
+; CHECK-NO-XO: adr [[REG:r[0-9]+]], [[LABEL:.?LCPI[0-9]+_[0-9]+]]
+; CHECK-NO-XO: vldr {{s[0-9]+}}, {{[[]}}[[REG]]{{[]]}}
+; CHECK-NO-XO-NOT: .rodata
+; CHECK-NO-XO: [[LABEL]]:
+; CHECK-NO-XO: .long   1335165689
+; CHECK-NO-XO: .long   1307470632
+
+; CHECK-XO-FLOAT-LABEL: lower_fpconst_select
+; CHECK-XO-FLOAT: movw [[REG:r[0-9]+]], :lower16:[[LABEL:.?LCP[0-9]+_[0-9]+]]
+; CHECK-XO-FLOAT: movt [[REG]], :upper16:[[LABEL]]
+; CHECK-XO-FLOAT: vldr {{s[0-9]+}}, {{[[]}}[[REG]]{{[]]}}
+; CHECK-XO-FLOAT: .rodata
+; CHECK-XO-FLOAT-NOT: .text
+; CHECK-XO-FLOAT: [[LABEL]]:
+; CHECK-XO-FLOAT: .long   1335165689
+; CHECK-XO-FLOAT: .long   1307470632
+
+; CHECK-XO-ROPI-LABEL: lower_fpconst_select
+; CHECK-XO-ROPI: movw [[REG:r[0-9]+]], :lower16:([[LABEL1:.?LCP[0-9]+_[0-9]+]]-([[LABEL2:.?LPC[0-9]+_[0-9]+]]+4))
+; CHECK-XO-ROPI: movt [[REG]], :upper16:([[LABEL1]]-([[LABEL2]]+4))
+; CHECK-XO-ROPI: [[LABEL2]]:
+; CHECK-XO-ROPI: vldr {{s[0-9]+}}, {{[[]}}[[REG]]{{[]]}}
+; CHECK-XO-ROPI: .rodata
+; CHECK-XO-ROPI-NOT: .text
+; CHECK-XO-ROPI: [[LABEL1]]:
+; CHECK-XO-ROPI: .long   1335165689
+; CHECK-XO-ROPI: .long   1307470632
+
+  %cmp = fcmp nnan oeq float %f, 0.000000e+00
+  %sel = select i1 %cmp, float 5.000000e+08, float 5.000000e+09
+  ret float %sel
+}