[Bitcode] Refactor attribute group writing to avoid getSlotAttributes

[rnk]

Summary

That API creates a temporary AttributeList to carry an index and a
single AttributeSet. We need to carry the index in addition to the set,
because that is how attribute groups are currently encoded.

NFC

[rnk]

• rebase after splitting out Attributes.cpp changes

[rnk]

• really rebase

[rnk]

I'm going to go ahead and land this. At first I wanted more eyes to confirm that this doesn't change functionality, but I'm pretty convinced now after staring at more.

[pcc]

I don't see anything wrong with this, but I don't have 100% confidence. I'll trust your judgement since you probably have more of attributes paged into your head at this point.

llvm/trunk/include/llvm/IR/Attributes.h

public:
   unsigned getNumSlots() const;
 
   /// \brief Return the index for the given slot.
   unsigned getSlotIndex(unsigned Slot) const;
 
   /// \brief Return the attributes at the given slot.
   AttributeList getSlotAttributes(unsigned Slot) const;
 
+  /// \brief Return the attributes at the given slot.
+  AttributeSet getSlotSet(unsigned Slot) const;
+
   void dump() const;
 };
 
 //===----------------------------------------------------------------------===//
 /// \class
 /// \brief Provide DenseMapInfo for AttributeList.
 template <> struct DenseMapInfo<AttributeList> {
   static inline AttributeList getEmptyKey() {

llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp

static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   case Attribute::None:
     llvm_unreachable("Can not encode none-attribute.");
   }
 
   llvm_unreachable("Trying to encode unknown attribute");
 }
 
 void ModuleBitcodeWriter::writeAttributeGroupTable() {
-  const std::vector<AttributeList> &AttrGrps = VE.getAttributeGroups();
+  const std::vector<ValueEnumerator::IndexAndAttrSet> &AttrGrps =
+      VE.getAttributeGroups();
   if (AttrGrps.empty()) return;
 
   Stream.EnterSubblock(bitc::PARAMATTR_GROUP_BLOCK_ID, 3);
 
   SmallVector<uint64_t, 64> Record;
-  for (unsigned i = 0, e = AttrGrps.size(); i != e; ++i) {
-    AttributeList AS = AttrGrps[i];
-    for (unsigned i = 0, e = AS.getNumSlots(); i != e; ++i) {
-      AttributeList A = AS.getSlotAttributes(i);
-
-      Record.push_back(VE.getAttributeGroupID(A));
-      Record.push_back(AS.getSlotIndex(i));
-
-      for (AttributeList::iterator I = AS.begin(0), E = AS.end(0); I != E;
-           ++I) {
-        Attribute Attr = *I;
-        if (Attr.isEnumAttribute()) {
-          Record.push_back(0);
-          Record.push_back(getAttrKindEncoding(Attr.getKindAsEnum()));
-        } else if (Attr.isIntAttribute()) {
-          Record.push_back(1);
-          Record.push_back(getAttrKindEncoding(Attr.getKindAsEnum()));
-          Record.push_back(Attr.getValueAsInt());
-        } else {
-          StringRef Kind = Attr.getKindAsString();
-          StringRef Val = Attr.getValueAsString();
+  for (ValueEnumerator::IndexAndAttrSet Pair : AttrGrps) {
+    unsigned AttrListIndex = Pair.first;
+    AttributeSet AS = Pair.second;
+    Record.push_back(VE.getAttributeGroupID(Pair));
+    Record.push_back(AttrListIndex);
+
+    for (Attribute Attr : AS) {
 
-          Record.push_back(Val.empty() ? 3 : 4);
-          Record.append(Kind.begin(), Kind.end());
-          Record.push_back(0);
-          if (!Val.empty()) {
-            Record.append(Val.begin(), Val.end());
-            Record.push_back(0);
-          }
-        }
-      }
 
-      Stream.EmitRecord(bitc::PARAMATTR_GRP_CODE_ENTRY, Record);
-      Record.clear();
-    }
-  }
 
   Stream.ExitBlock();
 }
 
 void ModuleBitcodeWriter::writeAttributeTable() {
-  const std::vector<AttributeList> &Attrs = VE.getAttributes();
+  const std::vector<AttributeList> &Attrs = VE.getAttributeLists();
   if (Attrs.empty()) return;
 
   Stream.EnterSubblock(bitc::PARAMATTR_BLOCK_ID, 3);
 
   SmallVector<uint64_t, 64> Record;
   for (unsigned i = 0, e = Attrs.size(); i != e; ++i) {
     const AttributeList &A = Attrs[i];
     for (unsigned i = 0, e = A.getNumSlots(); i != e; ++i)
-      Record.push_back(VE.getAttributeGroupID(A.getSlotAttributes(i)));
+      Record.push_back(
+          VE.getAttributeGroupID({A.getSlotIndex(i), A.getSlotSet(i)}));
 
     Stream.EmitRecord(bitc::PARAMATTR_CODE_ENTRY, Record);
     Record.clear();
   }
 
   Stream.ExitBlock();
 }
 
for (const Function &F : M) {
     //             unnamed_addr, prologuedata, dllstorageclass, comdat,
     //             prefixdata, personalityfn]
     Vals.push_back(StrtabBuilder.add(F.getName()));
     Vals.push_back(F.getName().size());
     Vals.push_back(VE.getTypeID(F.getFunctionType()));
     Vals.push_back(F.getCallingConv());
     Vals.push_back(F.isDeclaration());
     Vals.push_back(getEncodedLinkage(F));
-    Vals.push_back(VE.getAttributeID(F.getAttributes()));
+    Vals.push_back(VE.getAttributeListID(F.getAttributes()));
     Vals.push_back(Log2_32(F.getAlignment())+1);
     Vals.push_back(F.hasSection() ? SectionMap[F.getSection()] : 0);
     Vals.push_back(getEncodedVisibility(F));
     Vals.push_back(F.hasGC() ? GCMap[F.getGC()] : 0);
     Vals.push_back(getEncodedUnnamedAddr(F));
     Vals.push_back(F.hasPrologueData() ? (VE.getValueID(F.getPrologueData()) + 1)
                                        : 0);
     Vals.push_back(getEncodedDLLStorageClass(F));
case Instruction::Invoke: {
     const Value *Callee = II->getCalledValue();
     FunctionType *FTy = II->getFunctionType();
 
     if (II->hasOperandBundles())
       writeOperandBundles(II, InstID);
 
     Code = bitc::FUNC_CODE_INST_INVOKE;
 
-    Vals.push_back(VE.getAttributeID(II->getAttributes()));
+    Vals.push_back(VE.getAttributeListID(II->getAttributes()));
     Vals.push_back(II->getCallingConv() | 1 << 13);
     Vals.push_back(VE.getValueID(II->getNormalDest()));
     Vals.push_back(VE.getValueID(II->getUnwindDest()));
     Vals.push_back(VE.getTypeID(FTy));
     pushValueAndType(Callee, InstID, Vals);
 
     // Emit value #'s for the fixed parameters.
     for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i)
case Instruction::Call: {
     const CallInst &CI = cast<CallInst>(I);
     FunctionType *FTy = CI.getFunctionType();
 
     if (CI.hasOperandBundles())
       writeOperandBundles(&CI, InstID);
 
     Code = bitc::FUNC_CODE_INST_CALL;
 
-    Vals.push_back(VE.getAttributeID(CI.getAttributes()));
+    Vals.push_back(VE.getAttributeListID(CI.getAttributes()));
 
     unsigned Flags = getOptimizationFlags(&I);
     Vals.push_back(CI.getCallingConv() << bitc::CALL_CCONV |
                    unsigned(CI.isTailCall()) << bitc::CALL_TAIL |
                    unsigned(CI.isMustTailCall()) << bitc::CALL_MUSTTAIL |
                    1 << bitc::CALL_EXPLICIT_TYPE |
                    unsigned(CI.isNoTailCall()) << bitc::CALL_NOTAIL |
                    unsigned(Flags != 0) << bitc::CALL_FMF);

llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.h

 
 class ValueEnumerator {
 public:
   typedef std::vector<Type*> TypeList;
 
   // For each value, we remember its Value* and occurrence frequency.
   typedef std::vector<std::pair<const Value*, unsigned> > ValueList;
 
+  /// Attribute groups as encoded in bitcode are almost AttributeSets, but they
+  /// include the AttributeList index, so we have to track that in our map.
+  typedef std::pair<unsigned, AttributeSet> IndexAndAttrSet;
+
   UseListOrderStack UseListOrders;
 
 private:
   typedef DenseMap<Type*, unsigned> TypeMapType;
   TypeMapType TypeMap;
   TypeList Types;
 
   typedef DenseMap<const Value*, unsigned> ValueMapType;
struct MDRange {
 
     MDRange() {}
     explicit MDRange(unsigned First) : First(First) {}
   };
   SmallDenseMap<unsigned, MDRange, 1> FunctionMDInfo;
 
   bool ShouldPreserveUseListOrder;
 
-  typedef DenseMap<AttributeList, unsigned> AttributeGroupMapType;
+  typedef DenseMap<IndexAndAttrSet, unsigned> AttributeGroupMapType;
   AttributeGroupMapType AttributeGroupMap;
-  std::vector<AttributeList> AttributeGroups;
+  std::vector<IndexAndAttrSet> AttributeGroups;
 
-  typedef DenseMap<AttributeList, unsigned> AttributeMapType;
-  AttributeMapType AttributeMap;
-  std::vector<AttributeList> Attribute;
+  typedef DenseMap<AttributeList, unsigned> AttributeListMapType;
+  AttributeListMapType AttributeListMap;
+  std::vector<AttributeList> AttributeLists;
 
   /// GlobalBasicBlockIDs - This map memoizes the basic block ID's referenced by
   /// the "getGlobalBasicBlockID" method.
   mutable DenseMap<const BasicBlock*, unsigned> GlobalBasicBlockIDs;
 
   typedef DenseMap<const Instruction*, unsigned> InstructionMapType;
   InstructionMapType InstructionMap;
   unsigned InstructionCount;
unsigned getTypeID(Type *T) const {
     TypeMapType::const_iterator I = TypeMap.find(T);
     assert(I != TypeMap.end() && "Type not in ValueEnumerator!");
     return I->second-1;
   }
 
   unsigned getInstructionID(const Instruction *I) const;
   void setInstructionID(const Instruction *I);
 
-  unsigned getAttributeID(AttributeList PAL) const {
+  unsigned getAttributeListID(AttributeList PAL) const {
     if (PAL.isEmpty()) return 0;  // Null maps to zero.
-    AttributeMapType::const_iterator I = AttributeMap.find(PAL);
-    assert(I != AttributeMap.end() && "Attribute not in ValueEnumerator!");
+    AttributeListMapType::const_iterator I = AttributeListMap.find(PAL);
+    assert(I != AttributeListMap.end() && "Attribute not in ValueEnumerator!");
     return I->second;
   }
 
-  unsigned getAttributeGroupID(AttributeList PAL) const {
-    if (PAL.isEmpty()) return 0;  // Null maps to zero.
-    AttributeGroupMapType::const_iterator I = AttributeGroupMap.find(PAL);
+  unsigned getAttributeGroupID(IndexAndAttrSet Group) const {
+    if (!Group.second.hasAttributes())
+      return 0; // Null maps to zero.
+    AttributeGroupMapType::const_iterator I = AttributeGroupMap.find(Group);
     assert(I != AttributeGroupMap.end() && "Attribute not in ValueEnumerator!");
     return I->second;
   }
 
   /// getFunctionConstantRange - Return the range of values that corresponds to
   /// function-local constants.
   void getFunctionConstantRange(unsigned &Start, unsigned &End) const {
     Start = FirstFuncConstantID;
public:
   ArrayRef<const Metadata *> getNonMDStrings() const {
     return makeArrayRef(MDs).slice(NumModuleMDs).slice(NumMDStrings);
   }
 
   const TypeList &getTypes() const { return Types; }
   const std::vector<const BasicBlock*> &getBasicBlocks() const {
     return BasicBlocks;
   }
-  const std::vector<AttributeList> &getAttributes() const { return Attribute; }
-  const std::vector<AttributeList> &getAttributeGroups() const {
+  const std::vector<AttributeList> &getAttributeLists() const { return AttributeLists; }
+  const std::vector<IndexAndAttrSet> &getAttributeGroups() const {
     return AttributeGroups;
   }
 
   const ComdatSetType &getComdats() const { return Comdats; }
   unsigned getComdatID(const Comdat *C) const;
 
   /// getGlobalBasicBlockID - This returns the function-specific ID for the
   /// specified basic block.  This is relatively expensive information, so it

llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp

for (const Value *Op : C->operands()) {
     EnumerateOperandType(Op);
   }
 }
 
 void ValueEnumerator::EnumerateAttributes(AttributeList PAL) {
   if (PAL.isEmpty()) return;  // null is always 0.
 
   // Do a lookup.
-  unsigned &Entry = AttributeMap[PAL];
+  unsigned &Entry = AttributeListMap[PAL];
   if (Entry == 0) {
     // Never saw this before, add it.
-    Attribute.push_back(PAL);
-    Entry = Attribute.size();
+    AttributeLists.push_back(PAL);
+    Entry = AttributeLists.size();
   }
 
   // Do lookups for all attribute groups.
   for (unsigned i = 0, e = PAL.getNumSlots(); i != e; ++i) {
-    AttributeList AS = PAL.getSlotAttributes(i);
-    unsigned &Entry = AttributeGroupMap[AS];
+    IndexAndAttrSet Pair = {PAL.getSlotIndex(i), PAL.getSlotSet(i)};
+    unsigned &Entry = AttributeGroupMap[Pair];
     if (Entry == 0) {
-      AttributeGroups.push_back(AS);
+      AttributeGroups.push_back(Pair);
       Entry = AttributeGroups.size();
     }
   }
 }
 
 void ValueEnumerator::incorporateFunction(const Function &F) {
   InstructionCount = 0;
   NumModuleValues = Values.size();

llvm/trunk/lib/IR/Attributes.cpp

 }
 
 AttributeList AttributeList::getSlotAttributes(unsigned Slot) const {
   assert(pImpl && Slot < pImpl->getNumSlots() &&
          "Slot # out of range!");
   return pImpl->getSlotAttributes(Slot);
 }
 
+AttributeSet AttributeList::getSlotSet(unsigned Slot) const {
+  assert(pImpl && Slot < pImpl->getNumSlots() &&
+         "Slot # out of range!");
+  return pImpl->getSlotNode(Slot);
+}
+
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_DUMP_METHOD void AttributeList::dump() const {
   dbgs() << "PAL[\n";
 
   for (unsigned i = 0, e = getNumSlots(); i < e; ++i) {
     uint64_t Index = getSlotIndex(i);
     dbgs() << "  { ";
     if (Index == ~0U)