[PATCH] D12457: [Bitcode][Asm] Teach LLVM to read and write operand bundles.
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 11 17:12:00 PDT 2015
> On 2015-Sep-11, at 16:08, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
>
> > I think you can disable the `verify-uselistorder` run lines for now.
> > Something like:
> > --
> > RUN-PR24755: ...
> > --
> > makes sense to me (and then please document this in the PR so that we
> > remember to enable the tests when the PR gets fixed).
>
> Done. Now this change passes ninja check.
>
> I'll add the revision number to the PR when I check this in.
>
> > This uses a lot of new bitcode records. I think you can do it with just
> > one new function record, and simplify some of the logic in the process.
> >
> > Firstly, you can avoid the new forms of CALL/INVOKE by emitting the
> > BUNDLE records *before* the instructions instead of after. The reader
> > logic would be something like this:
> > --
> > SmallVector<Bundle, 8> Bundles;
> > while (1) {
> > Bitcode = ...;
> >
> > // Emit an error if bundles attached to invalid instruction.
> > if (!Bundles.empty()&&
> > Bitcode != BUNDLE&&
> > Bitcode != CALL&&
> > Bitcode != INVOKE)
> > return Error("Bundles attached to invalid instruction");
> >
> > // Old switch statement to read records.
> > switch (Bitcode) {
> > case BUNDLE:
> > Bundles.emplace_back(...);
> > break;
> > case CALL:
> > // ...
> > ... = CreateCall(..., Bundles);
> > break;
> > case ...
> > }
> > }
> > --
> > In the writer, you just emit the bundles before emitting the rest of the
> > instruction to accomplish this.
> >
> > Secondly, now that the bundles precede the instruction they're being
> > attached to, you don't need the BUNDLES_HEADER record. The number of
> > bundles is recorded implicitly.
> >
> > Thirdly, you can merge the BUNDLE_TAG and BUNDLE_CONTENTS records by
> > adding a field for the number of values.
> > --
> > // BUNDLE: [ V, V x vals, L x char ]
> > --
> >
> > Even better: I assume a module is only likely to have a small-ish set of
> > bundle tags that are reused? If so, you can add a BUNDLE_BLOCK (or
> > some such block) that defines the tags, and emit it at the top of the
> > module:
> > --
> > // BUNDLE_TAG: [ n x strchr ]
> > --
> > This implicitly numbers the tags by the order of the records.
> >
> > One benefit is that each tag is only serialized once, but this also
> > simplifies (and shrinks) the BUNDLE records, which can refer to the tags
> > by ID.
> > --
> > // BUNDLE: [ tag, n x vals ]
> > --
>
> Thanks for the detailed review!
>
> I've changed the bitcode reading / writing to use this prefix scheme.
> This second revision of the change introduces one new kind of block
> (OPERAND_BUNDLE_TAGS_BLOCK_ID) and one new kind of function code
> (FUNC_CODE_OPERAND_BUNDLE).
>
> >> enum UseListCodes {
> >> Index: docs/LangRef.rst
> >> ===================================================================
> >> --- docs/LangRef.rst
> >> +++ docs/LangRef.rst
> >> @@ -1438,7 +1438,32 @@
> >> the ELF x86-64 abi, but it can be disabled for some compilation
> >> units.
> >>
> >> -.. _moduleasm:
> >
> > Did you mean to delete this anchor?
>
> No, fixed.
>
> >> +Operand bundles are a generic mechanism intended to support
> >> +runtime-introspection like functionality for managed languages. The
> >
> > I would parse this better if "like" were hyphenated, as in "hyphen-like
> > syntax".
>
> Fixed.
>
> >
> >> +exact semantics of an operand bundle is dependent on the bundle tag.
> >
> > (I'm assuming you'll describe the tags here as they're added?)
>
> Yes. I also intend to add some more documentation before checking in
> (so I've added a TODO here).
>
The bitcode format LGTM now. Thanks for the work there.
A couple more minor code suggestions. Patch LGTM after that (assuming you
beef up the documentation); not sure if you're waiting on others too.
> Index: lib/IR/AsmWriter.cpp
> ===================================================================
> --- lib/IR/AsmWriter.cpp
> +++ lib/IR/AsmWriter.cpp
> @@ -2940,6 +2974,14 @@
> Out << ')';
> if (PAL.hasAttributes(AttributeSet::FunctionIndex))
> Out << " #" << Machine.getAttributeGroupSlot(PAL.getFnAttributes());
> +
> + if (CI->hasOperandBundles()) {
> + SmallVector<ConstOperandBundleSetUse, 2> Bundles;
> + for (unsigned i = 0, e = CI->getNumOperandBundles(); i != e; ++i)
> + Bundles.push_back(CI->getOperandBundle(i));
> + writeOperandBundles(Bundles);
> + }
> +
> } else if (const InvokeInst *II = dyn_cast<InvokeInst>(&I)) {
> Operand = II->getCalledValue();
> FunctionType *FTy = cast<FunctionType>(II->getFunctionType());
> @@ -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?
> +
> 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;
--
> +
> // 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?
> 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)
> InstructionList.push_back(I);
> cast<CallInst>(I)->setCallingConv(
> static_cast<CallingConv::ID>((~(1U << 14) & CCInfo) >> 1));
> @@ -4588,14 +4648,42 @@
> InstructionList.push_back(I);
> break;
> }
> +
> + case bitc::FUNC_CODE_OPERAND_BUNDLE: {
> + // A call or an invoke can be optionally prefixed with some variable
> + // number of operand bundle blocks. These blocks are read into
> + // OperandBundles and consumed at the next call or invoke instruction.
> +
> + SmallVector<Value *, 16> Inputs;
> + OperandBundles.emplace_back();
> + if (Record.size() < 1 || Record[0] >= BundleTags.size())
> + return error("Invalid record");
> +
> + OperandBundles.back().Tag = BundleTags[Record[0]];
> +
> + unsigned OpNum = 1;
> + while (OpNum != Record.size()) {
> + Value *Op;
> + if (getValueTypePair(Record, OpNum, NextValueNo, Op))
> + return error("Invalid record");
> + Inputs.push_back(Op);
> + }
> +
> + OperandBundles.back().Inputs = std::move(Inputs);
> + continue;
> + }
> }
>
> // Add instruction to end of current BB. If there is no current BB, reject
> // this file.
> if (!CurBB) {
> delete I;
> return error("Invalid instruction with no BB");
> }
> + if (!OperandBundles.empty()) {
> + delete I;
> + return error("Operand bundles found with no consumer");
> + }
Nice, this error handling (with the `continue`) looks nicer than what I
suggested.
> CurBB->getInstList().push_back(I);
>
> // If this was a terminator instruction, move to the next block.
> @@ -4612,6 +4700,9 @@
>
> OutOfRecordLoop:
>
> + if (!OperandBundles.empty())
> + return error("Operand bundles found with no consumer");
> +
> // Check the function list for unresolved values.
> if (Argument *A = dyn_cast<Argument>(ValueList.back())) {
More information about the llvm-commits
mailing list