Track globals promoted to coalesced const pool entries

[rinon]

Summary

Globals that are promoted to an ARM constant pool may alias with another
existing constant pool entry. We need to keep a reference to all globals that
were promoted to each constant pool value so that we can emit a distinct label
for each promoted global. These labels are necessary so that debug info can
refer to the promoted global without an undefined reference during linking.

[rinon]

Didn't realize llvm-commits needed to be added as a sub. Done now.

I just wanted to ping this patch and see if someone had time to review it. The bug seems fairly hard to trigger, but I have real code that can't build with LTO because of this problem (libXML2 linked statically into another lib, all built with LTO). I also have a reduced C test case, but the IR test is simpler.

Thanks in advance!

[compnerd]

Please trim down the test. I dont think that any of the debug info metadata is needed to validate the CPE is coalesced nor are the attributes.

lib/Target/ARM/ARMAsmPrinter.cpp

868–873

Shouldn't this be const auto *GV?

lib/Target/ARM/ARMConstantPoolValue.cpp

196

Please use C++ style casts (static_cast). Renaming the type doesn't offer much here, why not use auto?

211

This should probably be const auto * or const auto &.

lib/Target/ARM/ARMConstantPoolValue.h

179–180

If these aren't used right now, we could just inline them into the promotedGlobals call.

[rinon]

Address review comments

• Mark auto pointers explicitly const
• Remove unncessary iterator begin and end functions
• Remove debug info from IR test

[rinon]

Thanks for the review. I think I've addressed all comments.

I originally had the debug info in the IR test because I was also testing compiling the IR with clang to make sure the output binary could be linked. I've removed it now since it's not needed for the regression test.

[rinon]

Just wanted to ping this review and see if it's ready for commit.

[rinon]

Ping? Should be all ready to go.

[compnerd]

LG with the further test changes.

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

2

Please add an explicit triple. It makes debugging things easier (when the test fails, the command itself is printed, and the host may be different).

4–5

I think that you can drop these. I dont think that there is anything Linux specific about this. I think that thumbv7-unknown-none-eabi is a better target.

27–28

Are the attributes really necessary? Im pretty sure that you should be able to drop them and the annotations.

[rinon]

• Address review points
• Remove debug info from IR test
• Fix up test

[rinon]

Thanks, addressed test changes. Can someone commit this patch?

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

4–5

I used arm-eabi since it seems to be used in other ARM tests and this test doesn't depend on any thumb behavior. Is that alright?

[compnerd]

Sure, I can commit it on your behalf.

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

4–5

The other tests using it isn't the best argument. However, armv7-eabi is as good as thumbv7-eabi in this case.

[compnerd]

SVN r312692

lib/Target/ARM/ARMAsmPrinter.cpp

if (ACPV->isPromotedGlobal()) {
     // by debug information, and due to the way AsmPrinter is set up, the debug
     // info is immutable by the time we decide to promote globals to constant
     // pools. Because of this, we need to ensure we emit a symbol for the global
     // with private linkage (the default) so debug info can refer to it.
     //
     // However, if this global is promoted into several functions we must ensure
     // we don't try and emit duplicate symbols!
     auto *ACPC = cast<ARMConstantPoolConstant>(ACPV);
-    auto *GV = ACPC->getPromotedGlobal();
+    for (const auto *GV : ACPC->promotedGlobals()) {
       if (!EmittedPromotedGlobalLabels.count(GV)) {
         MCSymbol *GVSym = getSymbol(GV);
         OutStreamer->EmitLabel(GVSym);
         EmittedPromotedGlobalLabels.insert(GV);
       }
+    }
     return EmitGlobalConstant(DL, ACPC->getPromotedGlobalInit());
   }
 
   MCSymbol *MCSym;
   if (ACPV->isLSDA()) {
     MCSym = getCurExceptionSym();
   } else if (ACPV->isBlockAddress()) {
     const BlockAddress *BA =

lib/Target/ARM/ARMConstantPoolValue.h

 // This file implements the ARM specific constantpool value class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_ARM_ARMCONSTANTPOOLVALUE_H
 #define LLVM_LIB_TARGET_ARM_ARMCONSTANTPOOLVALUE_H
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/CodeGen/MachineConstantPool.h"
 #include "llvm/Support/Casting.h"
 #include <string>
 #include <vector>
 
 namespace llvm {
 
 class BlockAddress;
protected:
   template <typename Derived>
   int getExistingMachineCPValueImpl(MachineConstantPool *CP,
                                     unsigned Alignment) {
     unsigned AlignMask = Alignment - 1;
     const std::vector<MachineConstantPoolEntry> &Constants = CP->getConstants();
     for (unsigned i = 0, e = Constants.size(); i != e; ++i) {
       if (Constants[i].isMachineConstantPoolEntry() &&
           (Constants[i].getAlignment() & AlignMask) == 0) {
-        ARMConstantPoolValue *CPV =
-            (ARMConstantPoolValue *)Constants[i].Val.MachineCPVal;
+        auto *CPV =
+          static_cast<ARMConstantPoolValue*>(Constants[i].Val.MachineCPVal);
         if (Derived *APC = dyn_cast<Derived>(CPV))
           if (cast<Derived>(this)->equals(APC))
             return i;
       }
     }
 
     return -1;
   }
inline raw_ostream &operator<<(raw_ostream &O, const ARMConstantPoolValue &V) {
   V.print(O);
   return O;
 }
 
 /// ARMConstantPoolConstant - ARM-specific constant pool values for Constants,
 /// Functions, and BlockAddresses.
 class ARMConstantPoolConstant : public ARMConstantPoolValue {
   const Constant *CVal;         // Constant being loaded.
-  const GlobalVariable *GVar = nullptr;
+  SmallPtrSet<const GlobalVariable*, 1> GVars;
 
   ARMConstantPoolConstant(const Constant *C,
                           unsigned ID,
                           ARMCP::ARMCPKind Kind,
                           unsigned char PCAdj,
                           ARMCP::ARMCPModifier Modifier,
                           bool AddCurrentAddress);
   ARMConstantPoolConstant(Type *Ty, const Constant *C,
static ARMConstantPoolConstant *Create(const Constant *C, unsigned ID,
                                          ARMCP::ARMCPKind Kind,
                                          unsigned char PCAdj,
                                          ARMCP::ARMCPModifier Modifier,
                                          bool AddCurrentAddress);
 
   const GlobalValue *getGV() const;
   const BlockAddress *getBlockAddress() const;
 
-  const GlobalVariable *getPromotedGlobal() const {
-    return dyn_cast_or_null<GlobalVariable>(GVar);
+  typedef SmallPtrSet<const GlobalVariable *, 1>::iterator promoted_iterator;
+  iterator_range<promoted_iterator> promotedGlobals() {
+    return iterator_range<promoted_iterator>(GVars.begin(), GVars.end());
   }
 
   const Constant *getPromotedGlobalInit() const {
     return CVal;
   }
 
   int getExistingMachineCPValue(MachineConstantPool *CP,
                                 unsigned Alignment) override;
 

lib/Target/ARM/ARMConstantPoolValue.cpp

ARMConstantPoolConstant::ARMConstantPoolConstant(const Constant *C,
                                                  bool AddCurrentAddress)
   : ARMConstantPoolValue((Type*)C->getType(), ID, Kind, PCAdj, Modifier,
                          AddCurrentAddress),
     CVal(C) {}
 
 ARMConstantPoolConstant::ARMConstantPoolConstant(const GlobalVariable *GV,
                                                  const Constant *C)
     : ARMConstantPoolValue((Type *)C->getType(), 0, ARMCP::CPPromotedGlobal, 0,
-                           ARMCP::no_modifier, false),
-      CVal(C), GVar(GV) {}
+                           ARMCP::no_modifier, false), CVal(C) {
+  GVars.insert(GV);
+}
 
 ARMConstantPoolConstant *
 ARMConstantPoolConstant::Create(const Constant *C, unsigned ID) {
   return new ARMConstantPoolConstant(C, ID, ARMCP::CPValue, 0,
                                      ARMCP::no_modifier, false);
 }
 
 ARMConstantPoolConstant *

 }
 
 const BlockAddress *ARMConstantPoolConstant::getBlockAddress() const {
   return dyn_cast_or_null<BlockAddress>(CVal);
 }
 
 int ARMConstantPoolConstant::getExistingMachineCPValue(MachineConstantPool *CP,
                                                        unsigned Alignment) {
-  return getExistingMachineCPValueImpl<ARMConstantPoolConstant>(CP, Alignment);
+  int index =
+    getExistingMachineCPValueImpl<ARMConstantPoolConstant>(CP, Alignment);
+  if (index != -1) {
+    auto *CPV = static_cast<ARMConstantPoolValue*>(
+        CP->getConstants()[index].Val.MachineCPVal);
+    auto *Constant = cast<ARMConstantPoolConstant>(CPV);
+    Constant->GVars.insert(GVars.begin(), GVars.end());
+  }
+  return index;
 }
 
 bool ARMConstantPoolConstant::hasSameValue(ARMConstantPoolValue *ACPV) {
   const ARMConstantPoolConstant *ACPC = dyn_cast<ARMConstantPoolConstant>(ACPV);
   return ACPC && ACPC->CVal == CVal && ARMConstantPoolValue::hasSameValue(ACPV);
 }
 
 void ARMConstantPoolConstant::addSelectionDAGCSEId(FoldingSetNodeID &ID) {
   ID.AddPointer(CVal);
+  for (const auto *GV : GVars)
+    ID.AddPointer(GV);
   ARMConstantPoolValue::addSelectionDAGCSEId(ID);
 }
 
 void ARMConstantPoolConstant::print(raw_ostream &O) const {
   O << CVal->getName();
   ARMConstantPoolValue::print(O);
 }
 

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

+; RUN: llc -mtriple=arm-eabi -relocation-model=static -arm-promote-constant < %s | FileCheck %s
+
+@const1 = private unnamed_addr constant i32 0, align 4
+@const2 = private unnamed_addr constant i32 0, align 4
+
+; const1 and const2 both need labels for debug info, but will be coalesced into
+; a single constpool entry
+
+; CHECK-LABEL: @test1
+; CHECK-DAG: const1:
+; CHECK-DAG: const2:
+; CHECK: .fnend
+define void @test1() {
+  %1 = load i32, i32* @const1, align 4
+  call void @a(i32 %1)
+  %2 = load i32, i32* @const2, align 4
+  call void @a(i32 %2)
+  ret void
+}
+
+declare void @a(i32)