[llvm] r253446 - [OperandBundles] Tighten OperandBundleDef's interface; NFC
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 08:11:02 PST 2015
On Wed, Nov 18, 2015 at 12:30 AM, Sanjoy Das via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: sanjoy
> Date: Wed Nov 18 02:30:07 2015
> New Revision: 253446
>
> URL: http://llvm.org/viewvc/llvm-project?rev=253446&view=rev
> Log:
> [OperandBundles] Tighten OperandBundleDef's interface; NFC
>
> Modified:
> llvm/trunk/include/llvm/IR/InstrTypes.h
> llvm/trunk/lib/AsmParser/LLParser.cpp
> llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
> llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp
>
> Modified: llvm/trunk/include/llvm/IR/InstrTypes.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/InstrTypes.h?rev=253446&r1=253445&r2=253446&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/InstrTypes.h (original)
> +++ llvm/trunk/include/llvm/IR/InstrTypes.h Wed Nov 18 02:30:07 2015
> @@ -1155,21 +1155,30 @@ private:
> /// Unlike OperandBundleUse, OperandBundleDefT owns the memory it
> carries, and
> /// so it is possible to create and pass around "self-contained"
> instances of
> /// OperandBundleDef and ConstOperandBundleDef.
> -template <typename InputTy> struct OperandBundleDefT {
> +template <typename InputTy> class OperandBundleDefT {
> std::string Tag;
> std::vector<InputTy> Inputs;
>
> - OperandBundleDefT() {}
> - explicit OperandBundleDefT(StringRef Tag, const std::vector<InputTy>
> &Inputs)
> +public:
> + explicit OperandBundleDefT(StringRef Tag, std::vector<InputTy> &&Inputs)
>
As a general rule, you shouldn't pass by rvalue reference - instead pass by
value (that way you don't force callers to provide an rvalue when they may
not want to - but if they do provide one, it's efficiently moved, etc) -
similar below
> : Tag(Tag), Inputs(Inputs) {}
>
Without the std::move here ^ ("Inputs(std::move(Inputs))") this will be a
copy where a move was probably intended (similarly below)
>
> - explicit OperandBundleDefT(StringRef Tag, std::vector<InputTy> &&Inputs)
> + explicit OperandBundleDefT(std::string &&Tag, std::vector<InputTy>
> &&Inputs)
> : Tag(Tag), Inputs(Inputs) {}
>
> explicit OperandBundleDefT(const OperandBundleUse &OBU) {
> Tag = OBU.getTagName();
> Inputs.insert(Inputs.end(), OBU.Inputs.begin(), OBU.Inputs.end());
> }
> +
> + ArrayRef<InputTy> getInputs() const { return Inputs; }
>
Probably just name this ^ "inputs()" to be consistent with other range
accessors?
> +
> + typedef typename std::vector<InputTy>::const_iterator input_iterator;
> + size_t input_size() const { return Inputs.size(); }
> + input_iterator input_begin() const { return Inputs.begin(); }
> + input_iterator input_end() const { return Inputs.end(); }
>
Do you need direct begin/end? Rather than just using getInputs/inputs?
+
> + StringRef getTag() const { return Tag; }
> };
>
> typedef OperandBundleDefT<Value *> OperandBundleDef;
> @@ -1461,7 +1470,7 @@ protected:
> const unsigned BeginIndex) {
> auto It = static_cast<InstrTy *>(this)->op_begin() + BeginIndex;
> for (auto &B : Bundles)
> - It = std::copy(B.Inputs.begin(), B.Inputs.end(), It);
> + It = std::copy(B.input_begin(), B.input_end(), It);
>
Ah, so you do. I might still be happy writing that as "B.inputs().begin()"
though.. *shrug* (at some point we should just write range-based versions
of every algorithm we go to use so we could write "llvm::copy(B.inputs(),
It)" or similar)
>
> auto *ContextImpl = static_cast<InstrTy *>(this)->getContext().pImpl;
> auto BI = Bundles.begin();
> @@ -1470,9 +1479,9 @@ protected:
> for (auto &BOI : bundle_op_infos()) {
> assert(BI != Bundles.end() && "Incorrect allocation?");
>
> - BOI.Tag = ContextImpl->getOrInsertBundleTag(BI->Tag);
> + BOI.Tag = ContextImpl->getOrInsertBundleTag(BI->getTag());
> BOI.Begin = CurrentIndex;
> - BOI.End = CurrentIndex + BI->Inputs.size();
> + BOI.End = CurrentIndex + BI->input_size();
> CurrentIndex = BOI.End;
> BI++;
> }
> @@ -1486,7 +1495,7 @@ protected:
> static unsigned CountBundleInputs(ArrayRef<OperandBundleDef> Bundles) {
> unsigned Total = 0;
> for (auto &B : Bundles)
> - Total += B.Inputs.size();
> + Total += B.input_size();
> return Total;
> }
> };
>
> Modified: llvm/trunk/lib/AsmParser/LLParser.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/LLParser.cpp?rev=253446&r1=253445&r2=253446&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/AsmParser/LLParser.cpp (original)
> +++ llvm/trunk/lib/AsmParser/LLParser.cpp Wed Nov 18 02:30:07 2015
> @@ -1972,17 +1972,13 @@ bool LLParser::ParseOptionalOperandBundl
> if (ParseStringConstant(Tag))
> return true;
>
> - BundleList.emplace_back();
> - auto &OBI = BundleList.back();
> -
> - OBI.Tag = std::move(Tag);
> -
> if (ParseToken(lltok::lparen, "expected '(' in operand bundle"))
> return true;
>
> + std::vector<Value *> Inputs;
> while (Lex.getKind() != lltok::rparen) {
> // If this isn't the first input, we need a comma.
> - if (!OBI.Inputs.empty() &&
> + if (!Inputs.empty() &&
> ParseToken(lltok::comma, "expected ',' in input list"))
> return true;
>
> @@ -1990,9 +1986,11 @@ bool LLParser::ParseOptionalOperandBundl
> Value *Input = nullptr;
> if (ParseType(Ty) || ParseValue(Ty, Input, PFS))
> return true;
> - OBI.Inputs.push_back(Input);
> + Inputs.push_back(Input);
> }
>
> + BundleList.emplace_back(std::move(Tag), std::move(Inputs));
> +
> Lex.Lex(); // Lex the ')'.
> }
>
>
> Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=253446&r1=253445&r2=253446&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)
> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Nov 18 02:30:07
> 2015
> @@ -5120,10 +5120,7 @@ std::error_code BitcodeReader::parseFunc
> if (Record.size() < 1 || Record[0] >= BundleTags.size())
> return error("Invalid record");
>
> - OperandBundles.emplace_back();
> - OperandBundles.back().Tag = BundleTags[Record[0]];
> -
> - std::vector<Value *> &Inputs = OperandBundles.back().Inputs;
> + std::vector<Value *> Inputs;
>
> unsigned OpNum = 1;
> while (OpNum != Record.size()) {
> @@ -5133,6 +5130,7 @@ std::error_code BitcodeReader::parseFunc
> Inputs.push_back(Op);
> }
>
> + OperandBundles.emplace_back(BundleTags[Record[0]],
> std::move(Inputs));
> continue;
> }
> }
>
> Modified: llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp?rev=253446&r1=253445&r2=253446&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp Wed Nov 18 02:30:07
> 2015
> @@ -1195,7 +1195,7 @@ bool llvm::InlineFunction(CallSite CS, I
> MergedDeoptArgs.insert(MergedDeoptArgs.end(),
> ChildOB.Inputs.begin(),
> ChildOB.Inputs.end());
>
> - OpDefs.emplace_back("deopt", std::move(MergedDeoptArgs));
> + OpDefs.emplace_back(StringRef("deopt"),
> std::move(MergedDeoptArgs));
> }
>
> Instruction *NewI = nullptr;
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151118/c77ba532/attachment.html>
More information about the llvm-commits
mailing list