[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

180

typo: independent

184

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.

[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

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

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

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

214

• 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

2689

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.

lib/Target/ARM/ARMAsmPrinter.h

void emitInlineAsmEnd(const MCSubtargetInfo &StartInfo,
                         const MCSubtargetInfo *EndInfo) const override;
 
   void EmitJumpTableAddrs(const MachineInstr *MI);
   void EmitJumpTableInsts(const MachineInstr *MI);
   void EmitJumpTableTBInst(const MachineInstr *MI, unsigned OffsetWidth);
   void EmitInstruction(const MachineInstr *MI) override;
   bool runOnMachineFunction(MachineFunction &F) override;
 
-  void EmitConstantPool() override {
-    // we emit constant pools customly!
-  }
+  void EmitConstantPool() override;
   void EmitFunctionBodyEnd() override;
   void EmitFunctionEntryLabel() override;
   void EmitStartOfAsmFile(Module &M) override;
   void EmitEndOfAsmFile(Module &M) override;
   void EmitXXStructor(const DataLayout &DL, const Constant *CV) override;
   void EmitGlobalVariable(const GlobalVariable *GV) override;
   
   // lowerOperand - Convert a MachineOperand into the equivalent MCOperand.

lib/Target/ARM/ARMAsmPrinter.cpp

 
 #define DEBUG_TYPE "asm-printer"
 
 ARMAsmPrinter::ARMAsmPrinter(TargetMachine &TM,
                              std::unique_ptr<MCStreamer> Streamer)
     : AsmPrinter(TM, std::move(Streamer)), AFI(nullptr), MCP(nullptr),
       InConstantPool(false), OptimizationGoals(-1) {}
 
+// 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.
+void ARMAsmPrinter::EmitConstantPool() {
+  if (Subtarget->genExecuteOnly())
+    AsmPrinter::EmitConstantPool();
+}
+
 void ARMAsmPrinter::EmitFunctionBodyEnd() {
   // Make sure to terminate any constant pools that were at the end
   // of the function.
   if (!InConstantPool)
     return;
   InConstantPool = false;
   OutStreamer->EmitDataRegion(MCDR_DataRegionEnd);
 }
void ARMAsmPrinter::EmitInstruction(const MachineInstr *MI) {
   switch (Opc) {
   case ARM::t2MOVi32imm: llvm_unreachable("Should be lowered by thumb2it pass");
   case ARM::DBG_VALUE: llvm_unreachable("Should be handled by generic printing");
   case ARM::LEApcrel:
   case ARM::tLEApcrel:
   case ARM::t2LEApcrel: {
     // FIXME: Need to also handle globals and externals
     MCSymbol *CPISymbol = GetCPISymbol(MI->getOperand(1).getIndex());
+
+    // When generating execute-only code use MOVW/MOVT to materialize the
+    // address of a Constant Pool.
+    if (Subtarget->genExecuteOnly()) {
+      const MCExpr *SymExpr = MCSymbolRefExpr::create(CPISymbol, OutContext);
+      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::t2MOVi16)
+                     .addReg(MI->getOperand(0).getReg())
+                     .addExpr(ARMMCExpr::createLower16(SymExpr, OutContext))
+                     // Add predicate operands.
+                     .addImm(MI->getOperand(2).getImm())
+                     .addReg(MI->getOperand(3).getReg())
+                     // Add 's' bit operand (always reg0 for this)
+                     .addReg(MI->getOperand(0).getReg()));
+      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::t2MOVTi16)
+                     .addReg(MI->getOperand(0).getReg())
+                     .addReg(MI->getOperand(0).getReg())
+                     .addExpr(ARMMCExpr::createUpper16(SymExpr, OutContext))
+                     // Add predicate operands.
+                     .addImm(MI->getOperand(2).getImm())
+                     .addReg(MI->getOperand(3).getReg())
+                     // Add 's' bit operand (always reg0 for this)
+                     .addReg(MI->getOperand(0).getReg()));
+      return;
+    }
+
     EmitToStreamer(*OutStreamer, MCInstBuilder(MI->getOpcode() ==
                                                ARM::t2LEApcrel ? ARM::t2ADR
                   : (MI->getOpcode() == ARM::tLEApcrel ? ARM::tADR
                      : ARM::ADR))
       .addReg(MI->getOperand(0).getReg())
       .addExpr(MCSymbolRefExpr::create(CPISymbol, OutContext))
       // Add predicate operands.
       .addImm(MI->getOperand(2).getImm())
EmitToStreamer(*OutStreamer, MCInstBuilder(Opcode)
       .addImm(0)
       // Add predicate operands.
       .addImm(MI->getOperand(3).getImm())
       .addReg(MI->getOperand(4).getReg()));
 
     return;
   }
   case ARM::CONSTPOOL_ENTRY: {
+    // Handle this case in EmitConstantPool.
+    if (Subtarget->genExecuteOnly())
+      return;
+
     /// 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();
 

lib/Target/ARM/ARMISelLowering.cpp

static SDValue LowerConstantPool(SDValue Op, SelectionDAG &DAG) {
   else
     Res = DAG.getTargetConstantPool(CP->getConstVal(), PtrVT,
                                     CP->getAlignment());
   return DAG.getNode(ARMISD::Wrapper, dl, MVT::i32, Res);
 }
 
 unsigned ARMTargetLowering::getJumpTableEncoding() const {
   return MachineJumpTableInfo::EK_Inline;
 }
 
 SDValue ARMTargetLowering::LowerBlockAddress(SDValue Op,
                                              SelectionDAG &DAG) const {
   MachineFunction &MF = DAG.getMachineFunction();
   ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
   unsigned ARMPCLabelIndex = 0;
   SDLoc DL(Op);
   EVT PtrVT = getPointerTy(DAG.getDataLayout());
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);

test/CodeGen/ARM/constantfp.ll

 ; 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: .rodata
+; CHECK-XO-FLOAT: [[LABEL:.?LCPI[0-9]+_[0-9]+]]:
+; CHECK-XO-FLOAT: .long   1335165689
+; CHECK-XO-FLOAT: .long   1307470632
+; CHECK-XO-FLOAT: .section        .text,"axy",%progbits,unique,0
+; CHECK-XO-FLOAT: lower_fpconst_select:
+; CHECK-XO-FLOAT: movw [[REG:r[0-9]+]], :lower16:[[LABEL]]
+; CHECK-XO-FLOAT: movt [[REG]], :upper16:[[LABEL]]
+; CHECK-XO-FLOAT: vldr {{s[0-9]+}}, {{[[]}}[[REG]]{{[]]}}
+
+  %cmp = fcmp nnan oeq float %f, 0.000000e+00
+  %sel = select i1 %cmp, float 5.000000e+08, float 5.000000e+09
+  ret float %sel
+}