[OpaquePtr][BitcodeWriter] Handle attributes with types

[aeubanks]

Summary

For example, byval.

Skip the type attribute auto-upgrade if we already have the type.

I've actually seen this error of the ValueEnumerator missing a type
attribute's type in a non-opaque pointer context.

[aeubanks]

use better check

[nikic]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3337

Shouldn't this only check for a type for the given kind, rather than all attribute kinds? I guess it doesn't matter if all of these are mutually incompatible.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

1049

Shouldn't this be inside the Entry == 0 condition, as it's something we only need to do once, not each time the AttributeSet is seen?

[aeubanks]

only enumerate type attributes once

[nikic]

LGTM

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
   // argument's pointee type. There should be no opaque pointers where the byval
   // type is implicit.
   for (unsigned i = 0; i != Func->arg_size(); ++i) {
     for (Attribute::AttrKind Kind : {Attribute::ByVal, Attribute::StructRet,
                                      Attribute::InAlloca}) {
       if (!Func->hasParamAttribute(i, Kind))
         continue;
 
+      if (Func->getParamByValType(i) || Func->getParamInAllocaType(i) ||
+          Func->getParamStructRetType(i))
+        continue;
+
       Func->removeParamAttr(i, Kind);
 
       Type *PTy = cast<FunctionType>(FTy)->getParamType(i);
       Type *PtrEltTy = cast<PointerType>(PTy)->getElementType();
       Attribute NewAttr;
       switch (Kind) {
       case Attribute::ByVal:
         NewAttr = Attribute::getWithByValType(Context, PtrEltTy);

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

for (unsigned i = PAL.index_begin(), e = PAL.index_end(); i != e; ++i) {
     if (!AS.hasAttributes())
       continue;
     IndexAndAttrSet Pair = {i, AS};
     unsigned &Entry = AttributeGroupMap[Pair];
     if (Entry == 0) {
       AttributeGroups.push_back(Pair);
       Entry = AttributeGroups.size();
     }
+    for (Attribute Attr : AS) {
+      if (Attr.isTypeAttribute())
+        EnumerateType(Attr.getValueAsType());
+    }
   }
 }
 
 void ValueEnumerator::incorporateFunction(const Function &F) {
   InstructionCount = 0;
   NumModuleValues = Values.size();
 
   // Add global metadata to the function block.  This doesn't include

llvm/test/Assembler/opaque-ptr.ll

 continue:
   ret void
 
 cleanup:
   landingpad {}
     cleanup
   ret void
 }
+
+; CHECK: define void @byval(ptr byval({ i32, i32 }) %0)
+define void @byval(ptr byval({ i32, i32 }) %0) {
+  ret void
+}