[ARM] Fix correctness checks in promoteToConstantPool.

[efriedma]

Summary

Correctly check for relocations in the constant to promote. And don't allow promoting a constant multiple times.

This partially fixes https://bugs.llvm.org/show_bug.cgi?id=32780 ; it's not a complete fix because we also need to prevent ARMConstantIslands from cloning the constant.

(-arm-promote-constant is currently off by default, and it stays off with this patch. I'll look into turning it on again when all the known issues are fixed.)

[dmgreen]

Hello. Good to see this getting some love.

Sorry for the delay. I wanted to get D51410 in and see check what affect these had on performance. (And I've been busy with the regressions we've had come in lately). Results look good, although there's still some 16 byte alignment cases I need to look into.

Did you ever see D36787? It's almost certainly not worth looking at very much. I remember it being a bit over-complicated with constant pool entries pointing to other constant pool entries. If it helps, feel free to take what might be useful. I'm kind of hoping there is a better way than that though.

I will try and stare more closely at this tomorrow, if noone else takes a look earlier.

[efriedma]

Did you ever see D36787? It's almost certainly not worth looking at very much. I remember it being a bit over-complicated with constant pool entries pointing to other constant pool entries. If it helps, feel free to take what might be useful. I'm kind of hoping there is a better way than that though.

No, I don't think I ever saw it (or at least, I don't remember seeing it).

Looking at it, yes, it does seem a little more complicated than I would have hoped for. But I'm not sure what a better solution would look like... I mean, if there are multiple tLEApcrel instructions referring to a given constant, ARMConstantIslands will be forced to do something like D36787 in general. (By "like D36787", I mean computing the address of the constant using some sequence that isn't a tLEApcrel; I guess instead of computing the address like the address of a global, you could "outline" the computation, but that's equally awkward).

I guess the alternative is to avoid cloning. If, in isel, we can prove that all references to the constant in question are lowered to a single SelectionDAG node (essentially, all references are in the same BasicBlock), we could lower that to some special tLEApcrel_nodup that's marked isNotDuplicable, and then ARMConstantIslands will ensure it's in range without cloning.

[dmgreen]

Constant Island pass has a bit of a reputation for being the source of difficult bugs. With enough Mir testing, perhaps that doesn't need to be the case though. The general idea of turning the adr's into ldr's seems like a good one, maybe it just needs some better machinary in the pass to handle the pool entries more easily?

This here looks like two separate changes, both of which look fine to me.

lib/Target/ARM/ARMISelLowering.cpp

while (!Worklist.empty()) {
 
     auto *I = dyn_cast<Instruction>(U);
     if (!I || I->getParent()->getParent() != F)
       return false;
   }
   return true;
 }
 
-/// Return true if all users of V are within some (any) function, looking through
-/// ConstantExprs. In other words, are there any global constant users?
-static bool allUsersAreInFunctions(const Value *V) {
-  SmallVector<const User*,4> Worklist;
-  for (auto *U : V->users())
-    Worklist.push_back(U);
-  while (!Worklist.empty()) {
-    auto *U = Worklist.pop_back_val();
-    if (isa<ConstantExpr>(U)) {
-      for (auto *UU : U->users())
-        Worklist.push_back(UU);
-      continue;
-    }
-
-    if (!isa<Instruction>(U))
-      return false;
-  }
-  return true;
-}
-
-// Return true if T is an integer, float or an array/vector of either.
-static bool isSimpleType(Type *T) {
-  if (T->isIntegerTy() || T->isFloatingPointTy())
-    return true;
-  Type *SubT = nullptr;
-  if (T->isArrayTy())
-    SubT = T->getArrayElementType();
-  else if (T->isVectorTy())
-    SubT = T->getVectorElementType();
-  else
-    return false;
-  return SubT->isIntegerTy() || SubT->isFloatingPointTy();
-}
-
-static SDValue promoteToConstantPool(const GlobalValue *GV, SelectionDAG &DAG,
+static SDValue promoteToConstantPool(const ARMTargetLowering *TLI,
+                                     const GlobalValue *GV, SelectionDAG &DAG,
                                      EVT PtrVT, const SDLoc &dl) {
   // If we're creating a pool entry for a constant global with unnamed address,
   // and the global is small enough, we can emit it inline into the constant pool
   // to save ourselves an indirection.
   //
   // This is a win if the constant is only used in one function (so it doesn't
   // need to be duplicated) or duplicating the constant wouldn't increase code
   // size (implying the constant is no larger than 4 bytes).
if (!EnableConstpoolPromotion ||
       return SDValue();
 
   auto *GVar = dyn_cast<GlobalVariable>(GV);
   if (!GVar || !GVar->hasInitializer() ||
       !GVar->isConstant() || !GVar->hasGlobalUnnamedAddr() ||
       !GVar->hasLocalLinkage())
     return SDValue();
 
-  // Ensure that we don't try and inline any type that contains pointers. If
-  // we inline a value that contains relocations, we move the relocations from
-  // .data to .text which is not ideal.
+  // If we inline a value that contains relocations, we move the relocations
+  // from .data to .text. This is not allowed in position-independent code.
   auto *Init = GVar->getInitializer();
-  if (!isSimpleType(Init->getType()))
+  if ((TLI->isPositionIndependent() || TLI->getSubtarget()->isROPI()) &&
+      Init->needsRelocation())
     return SDValue();
 
   // The constant islands pass can only really deal with alignment requests
   // <= 4 bytes and cannot pad constants itself. Therefore we cannot promote
   // any type wanting greater alignment requirements than 4 bytes. We also
   // can only promote constants that are multiples of 4 bytes in size or
   // are paddable to a multiple of 4. Currently we only try and pad constants
   // that are strings for simplicity.
static SDValue promoteToConstantPool(const ARMTargetLowering *TLI,
   // may fail to converge. If we haven't promoted this global yet (it may have
   // multiple uses), and promoting it would increase the constant pool size (Sz
   // > 4), ensure we have space to do so up to MaxTotal.
   if (!AFI->getGlobalsPromotedToConstantPool().count(GVar) && Size > 4)
     if (AFI->getPromotedConstpoolIncrease() + PaddedSize - 4 >=
         ConstpoolPromotionMaxTotal)
       return SDValue();
 
-  // This is only valid if all users are in a single function OR it has users
-  // in multiple functions but it no larger than a pointer. We also check if
-  // GVar has constant (non-ConstantExpr) users. If so, it essentially has its
-  // address taken.
-  if (!allUsersAreInFunction(GVar, &F) &&
-      !(Size <= 4 && allUsersAreInFunctions(GVar)))
+  // This is only valid if all users are in a single function; we can't clone
+  // the constant in general. The LLVM IR unnamed_addr allows merging
+  // constants, but not cloning them.
+  //
+  // We could potentially allow cloning if we could prove all uses of the
+  // constant in the current function don't care about the address, like
+  // printf format strings. But that isn't implemented for now.
+  if (!allUsersAreInFunction(GVar, &F))
     return SDValue();
 
   // We're going to inline this global. Pad it out if needed.
   if (RequiredPadding != 4) {
     StringRef S = CDAInit->getAsString();
 
     SmallVector<uint8_t,16> V(S.size());
     std::copy(S.bytes_begin(), S.bytes_end(), V.begin());
SDValue ARMTargetLowering::LowerGlobalAddressELF(SDValue Op,
   EVT PtrVT = getPointerTy(DAG.getDataLayout());
   SDLoc dl(Op);
   const GlobalValue *GV = cast<GlobalAddressSDNode>(Op)->getGlobal();
   const TargetMachine &TM = getTargetMachine();
   bool IsRO = isReadOnly(GV);
 
   // promoteToConstantPool only if not generating XO text section
   if (TM.shouldAssumeDSOLocal(*GV->getParent(), GV) && !Subtarget->genExecuteOnly())
-    if (SDValue V = promoteToConstantPool(GV, DAG, PtrVT, dl))
+    if (SDValue V = promoteToConstantPool(this, GV, DAG, PtrVT, dl))
       return V;
 
   if (isPositionIndependent()) {
     bool UseGOT_PREL = !TM.shouldAssumeDSOLocal(*GV->getParent(), GV);
     SDValue G = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0,
                                            UseGOT_PREL ? ARMII::MO_GOT : 0);
     SDValue Result = DAG.getNode(ARMISD::WrapperPIC, dl, PtrVT, G);
     if (UseGOT_PREL)

test/CodeGen/ARM/constantpool-promote-dbg.ll

 ; RUN: llc -relocation-model=static -arm-promote-constant < %s | FileCheck %s
 
 target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
 target triple = "thumbv7m--linux-gnu"
 
 @.str = private unnamed_addr constant [4 x i8] c"abc\00", align 1
 
 ; CHECK-LABEL: fn1
-; CHECK: .str:
+; CHECK: .long .L.str
 define arm_aapcscc i8* @fn1() local_unnamed_addr #0 !dbg !8 {
 entry:
   ret i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), !dbg !14
 }
 
 ; CHECK-LABEL: fn2
-; CHECK-NOT: .str:
+; CHECK: .long .L.str
 define arm_aapcscc i8* @fn2() local_unnamed_addr #0 !dbg !15 {
 entry:
   ret i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 1), !dbg !16
 }
 
 attributes #0 = { minsize norecurse nounwind optsize readnone "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="cortex-m3" "target-features"="+hwdiv,+soft-float,-crypto,-neon" "unsafe-fp-math"="false" "use-soft-float"="true" }
 
 !llvm.dbg.cu = !{!0}

test/CodeGen/ARM/constantpool-promote.ll

-; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=static -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
-; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=pic -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
+; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=static -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM,CHECK-STATIC
+; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=pic -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM,CHECK-PIC
 ; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=ropi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
 ; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=rwpi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
 ; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=static -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
 ; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=pic -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
 ; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=ropi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
 ; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=rwpi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
 ; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=static -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
 ; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=pic -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
 ; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=ropi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
 ; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=rwpi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
 
 @.str = private unnamed_addr constant [2 x i8] c"s\00", align 1
 @.str1 = private unnamed_addr constant [69 x i8] c"this string is far too long to fit in a literal pool by far and away\00", align 1
 @.str2 = private unnamed_addr constant [27 x i8] c"this string is just right!\00", align 1
 @.str3 = private unnamed_addr constant [26 x i8] c"this string is used twice\00", align 1
 @.str4 = private unnamed_addr constant [29 x i8] c"same string in two functions\00", align 1
+@.str5 = private unnamed_addr constant [2 x i8] c"s\00", align 1
 @.arr1 = private unnamed_addr constant [2 x i16] [i16 3, i16 4], align 2
 @.arr2 = private unnamed_addr constant [2 x i16] [i16 7, i16 8], align 2
 @.arr3 = private unnamed_addr constant [2 x i16*] [i16* null, i16* null], align 4
 @.ptr = private unnamed_addr constant [2 x i16*] [i16* getelementptr inbounds ([2 x i16], [2 x i16]* @.arr2, i32 0, i32 0), i16* null], align 2
 @.arr4 = private unnamed_addr constant [2 x i16] [i16 3, i16 4], align 16
+@.arr5 = private unnamed_addr constant [2 x i16] [i16 3, i16 4], align 2
 @.zerosize = private unnamed_addr constant [0 x i16] zeroinitializer, align 4
 @implicit_alignment_vector = private unnamed_addr constant <4 x i32> <i32 1, i32 2, i32 3, i32 4>
 
 ; CHECK-LABEL: @test1
 ; CHECK: adr r0, [[x:.*]]
 ; CHECK: [[x]]:
 ; CHECK: .asciz "s\000\000"
 define void @test1() #0 {

 }
 
 define void @test5b() #0 {
   tail call void @b(i8* getelementptr inbounds ([29 x i8], [29 x i8]* @.str4, i32 0, i32 0)) #2
   ret void
 }
 
 ; CHECK-LABEL: @test6a
-; CHECK: adr r0, [[x:.*]]
-; CHECK: [[x]]:
-; CHECK: .short 3
-; CHECK: .short 4
+; CHECK: L.arr1
 define void @test6a() #0 {
   tail call void @c(i16* getelementptr inbounds ([2 x i16], [2 x i16]* @.arr1, i32 0, i32 0)) #2
   ret void
 }
 
 ; CHECK-LABEL: @test6b
-; CHECK: adr r0, [[x:.*]]
-; CHECK: [[x]]:
-; CHECK: .short 3
-; CHECK: .short 4
+; CHECK: L.arr1
 define void @test6b() #0 {
   tail call void @c(i16* getelementptr inbounds ([2 x i16], [2 x i16]* @.arr1, i32 0, i32 0)) #2
   ret void
 }
 
 ; This shouldn't be promoted, as the string is used by another global.
 ; CHECK-LABEL: @test7
 ; CHECK-NOT: adr
 define void @test7() #0 {
   tail call void @c(i16* getelementptr inbounds ([2 x i16], [2 x i16]* @.arr2, i32 0, i32 0)) #2
   ret void  
 }
 
-; This shouldn't be promoted, because the array contains pointers.
+; This can be promoted; it contains pointers, but they don't need relocations.
 ; CHECK-LABEL: @test8
-; CHECK-NOT: .zero
+; CHECK: .zero
 ; CHECK: .fnend
 define void @test8() #0 {
   %a = load i16*, i16** getelementptr inbounds ([2 x i16*], [2 x i16*]* @.arr3, i32 0, i32 0)
   tail call void @c(i16* %a) #2
   ret void
 }
 
+; This can't be promoted in PIC mode because it contains pointers to other globals.
+; CHECK-LABEL: @test8a
+; CHECK-STATIC: .long .L.arr2
+; CHECK-PIC: .long .L.ptr
+; CHECK: .fnend
+define void @test8a() #0 {
+  %a = load i16*, i16** getelementptr inbounds ([2 x i16*], [2 x i16*]* @.ptr, i32 0, i32 0)
+  tail call void @c(i16* %a) #2
+  ret void
+}
+
 @fn1.a = private unnamed_addr constant [4 x i16] [i16 4, i16 0, i16 0, i16 0], align 2
 @fn2.a = private unnamed_addr constant [8 x i8] [i8 4, i8 0, i8 0, i8 0, i8 23, i8 0, i8 6, i8 0], align 1
 
 ; Just check these don't crash.
 define void @fn1() "target-features"="+strict-align"  {
 entry:
   %a = alloca [4 x i16], align 2
   %0 = bitcast [4 x i16]* %a to i8*

 ; CHECK-LABEL: @test10
 ; CHECK-V6M: adr r{{[0-9]*}}, [[x:.*]]
 ; CHECK-V6M: [[x]]:
 ; CHECK-V6M: .asciz "s\000\000"
 ; CHECK-V7: ldrb{{(.w)?}} r{{[0-9]*}}, [[x:.*]]
 ; CHECK-V7: [[x]]:
 ; CHECK-V7: .asciz "s\000\000"
 define void @test10(i8* %a) local_unnamed_addr #0 {
-  call void @llvm.memmove.p0i8.p0i8.i32(i8* align 1 %a, i8* align 1 getelementptr inbounds ([2 x i8], [2 x i8]* @.str, i32 0, i32 0), i32 1, i1 false)
+  call void @llvm.memmove.p0i8.p0i8.i32(i8* align 1 %a, i8* align 1 getelementptr inbounds ([2 x i8], [2 x i8]* @.str5, i32 0, i32 0), i32 1, i1 false)
   ret void
 }
 
 ; CHECK-LABEL: @test11
 ; CHECK-V6M: adr r{{[0-9]*}}, [[x:.*]]
 ; CHECK-V6M: [[x]]:
 ; CHECK-V6M: .short 3
 ; CHECK-V6M: .short 4
 ; CHECK-V7THUMB: ldrh{{(.w)?}} r{{[0-9]*}}, [[x:.*]]
 ; CHECK-V7THUMB: [[x]]:
 ; CHECK-V7THUMB: .short 3
 ; CHECK-V7THUMB: .short 4
 ; CHECK-V7ARM: adr r{{[0-9]*}}, [[x:.*]]
 ; CHECK-V7ARM: [[x]]:
 ; CHECK-V7ARM: .short 3
 ; CHECK-V7ARM: .short 4
 define void @test11(i16* %a) local_unnamed_addr #0 {
-  call void @llvm.memmove.p0i16.p0i16.i32(i16* align 2 %a, i16* align 2 getelementptr inbounds ([2 x i16], [2 x i16]* @.arr1, i32 0, i32 0), i32 2, i1 false)
+  call void @llvm.memmove.p0i16.p0i16.i32(i16* align 2 %a, i16* align 2 getelementptr inbounds ([2 x i16], [2 x i16]* @.arr5, i32 0, i32 0), i32 2, i1 false)
   ret void
 }
 
 ; Promotion only works with globals with alignment 4 or less; a vector has
 ; implicit alignment 16.
 ; CHECK-LABEL: @test12
 ; CHECK-NOT: adr
 define void @test12() local_unnamed_addr #0 {