[PATCH] D12457: [Bitcode][Asm] Teach LLVM to read and write operand bundles.

[Sanjoy Das via llvm-commits]

Duncan P. N. Exon Smith wrote:

>> @@ -2974,6 +3016,13 @@
>>       if (PAL.hasAttributes(AttributeSet::FunctionIndex))
>>         Out<<  " #"<<  Machine.getAttributeGroupSlot(PAL.getFnAttributes());
>>
>> +    if (II->hasOperandBundles()) {
>> +      SmallVector<ConstOperandBundleSetUse, 2>  Bundles;
>> +      for (unsigned i = 0, e = II->getNumOperandBundles(); i != e; ++i)
>> +        Bundles.push_back(II->getOperandBundle(i));
>> +      writeOperandBundles(Bundles);
>> +    }
>
> Seems like unfortunate code duplication between calls and invokes.  Is
> there some way to remove it?

I think I can just use a CallSite here; I'll give it a try on Monday.

>
>> +
>>       Out<<  "\n          to ";
>>       writeOperand(II->getNormalDest(), true);
>>       Out<<  " unwind ";
>> Index: lib/Bitcode/Reader/BitcodeReader.cpp
>> ===================================================================
>> --- lib/Bitcode/Reader/BitcodeReader.cpp
>> +++ lib/Bitcode/Reader/BitcodeReader.cpp
>> @@ -3411,6 +3459,8 @@
>>       return nullptr;
>>     };
>>
>> +  std::vector<OperandBundleInfo>  OperandBundles;
>
> Why is this not the following?
> --
> SmallVector<OperandBundleSetDef, 2>  Bundles;

OperandBundleSetDef tracks the input set as an ArrayRef, so I need some 
backing store.  OperandBundleInfo contains that backing store in the 
form of an std::vector.  But on second thought, I think it makes more 
sense to have OperandBundleSetDef use an std::vector<> or SmallVector<> 
instead of an ArrayRef<> -- for an OperandBundleSetUse the backing store 
is the Use& array of the User, but for an OperandBundleSetDef it makes 
sense to include the backing store in the structure itself.

The collection of OperandBundleInfo's itself should have been a 
SmallVector, not a std::vector -- that's just an omission.

I'll upload a patch fixing both these issues next week.

> --
>
>> +
>>     // Read all the records.
>>     SmallVector<uint64_t, 64>  Record;
>>     while (1) {
>> @@ -4180,7 +4230,12 @@
>>           }
>>         }
>>
>> -      I = InvokeInst::Create(Callee, NormalBB, UnwindBB, Ops);
>> +      SmallVector<OperandBundleSetDef, 2>  Bundles;
>> +      for (auto&OBI : OperandBundles)
>> +        Bundles.emplace_back(OBI.Tag, OBI.Inputs);
>> +      OperandBundles.clear();
>> +
>> +      I = InvokeInst::Create(Callee, NormalBB, UnwindBB, Ops, Bundles);
>
> I'd expect this simpler diff:
> --
> -      I = InvokeInst::Create(Callee, NormalBB, UnwindBB, Ops);
> +      I = InvokeInst::Create(Callee, NormalBB, UnwindBB, Ops, Bundles);
> +      Bundles.clear();
> --
> Is there a reason that isn't possible?

Same excuse about OperandBundleSetDef not having a backing store as above.

>
>>         InstructionList.push_back(I);
>>         cast<InvokeInst>(I)
>>             ->setCallingConv(static_cast<CallingConv::ID>(~(1U<<  13)&  CCInfo));
>> @@ -4563,7 +4618,12 @@
>>           }
>>         }
>>
>> -      I = CallInst::Create(FTy, Callee, Args);
>> +      SmallVector<OperandBundleSetDef, 2>  Bundles;
>> +      for (auto&OBI : OperandBundles)
>> +        Bundles.emplace_back(OBI.Tag, OBI.Inputs);
>> +      OperandBundles.clear();
>> +
>> +      I = CallInst::Create(FTy, Callee, Args, Bundles);
>
> (same here)

(same response)

Thanks!
-- Sanjoy