<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 18, 2015 at 12:30 AM, Sanjoy Das via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: sanjoy<br>
Date: Wed Nov 18 02:30:07 2015<br>
New Revision: 253446<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=253446&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=253446&view=rev</a><br>
Log:<br>
[OperandBundles] Tighten OperandBundleDef's interface; NFC<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/IR/InstrTypes.h<br>
llvm/trunk/lib/AsmParser/LLParser.cpp<br>
llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/IR/InstrTypes.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/InstrTypes.h?rev=253446&r1=253445&r2=253446&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/InstrTypes.h?rev=253446&r1=253445&r2=253446&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/IR/InstrTypes.h (original)<br>
+++ llvm/trunk/include/llvm/IR/InstrTypes.h Wed Nov 18 02:30:07 2015<br>
@@ -1155,21 +1155,30 @@ private:<br>
/// Unlike OperandBundleUse, OperandBundleDefT owns the memory it carries, and<br>
/// so it is possible to create and pass around "self-contained" instances of<br>
/// OperandBundleDef and ConstOperandBundleDef.<br>
-template <typename InputTy> struct OperandBundleDefT {<br>
+template <typename InputTy> class OperandBundleDefT {<br>
std::string Tag;<br>
std::vector<InputTy> Inputs;<br>
<br>
- OperandBundleDefT() {}<br>
- explicit OperandBundleDefT(StringRef Tag, const std::vector<InputTy> &Inputs)<br>
+public:<br>
+ explicit OperandBundleDefT(StringRef Tag, std::vector<InputTy> &&Inputs)<br></blockquote><div><br></div><div>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</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
: Tag(Tag), Inputs(Inputs) {}<br></blockquote><div><br></div><div>Without the std::move here ^ ("Inputs(std::move(Inputs))") this will be a copy where a move was probably intended (similarly below)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- explicit OperandBundleDefT(StringRef Tag, std::vector<InputTy> &&Inputs)<br>
+ explicit OperandBundleDefT(std::string &&Tag, std::vector<InputTy> &&Inputs)<br>
: Tag(Tag), Inputs(Inputs) {}<br>
<br>
explicit OperandBundleDefT(const OperandBundleUse &OBU) {<br>
Tag = OBU.getTagName();<br>
Inputs.insert(Inputs.end(), OBU.Inputs.begin(), OBU.Inputs.end());<br>
}<br>
+<br>
+ ArrayRef<InputTy> getInputs() const { return Inputs; }<br></blockquote><div><br></div><div>Probably just name this ^ "inputs()" to be consistent with other range accessors?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ typedef typename std::vector<InputTy>::const_iterator input_iterator;<br>
+ size_t input_size() const { return Inputs.size(); }<br>
+ input_iterator input_begin() const { return Inputs.begin(); }<br>
+ input_iterator input_end() const { return Inputs.end(); }<br></blockquote><div><br>Do you need direct begin/end? Rather than just using getInputs/inputs?<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ StringRef getTag() const { return Tag; }<br>
};<br>
<br>
typedef OperandBundleDefT<Value *> OperandBundleDef;<br>
@@ -1461,7 +1470,7 @@ protected:<br>
const unsigned BeginIndex) {<br>
auto It = static_cast<InstrTy *>(this)->op_begin() + BeginIndex;<br>
for (auto &B : Bundles)<br>
- It = std::copy(B.Inputs.begin(), B.Inputs.end(), It);<br>
+ It = std::copy(B.input_begin(), B.input_end(), It);<br></blockquote><div><br></div><div>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
auto *ContextImpl = static_cast<InstrTy *>(this)->getContext().pImpl;<br>
auto BI = Bundles.begin();<br>
@@ -1470,9 +1479,9 @@ protected:<br>
for (auto &BOI : bundle_op_infos()) {<br>
assert(BI != Bundles.end() && "Incorrect allocation?");<br>
<br>
- BOI.Tag = ContextImpl->getOrInsertBundleTag(BI->Tag);<br>
+ BOI.Tag = ContextImpl->getOrInsertBundleTag(BI->getTag());<br>
BOI.Begin = CurrentIndex;<br>
- BOI.End = CurrentIndex + BI->Inputs.size();<br>
+ BOI.End = CurrentIndex + BI->input_size();<br>
CurrentIndex = BOI.End;<br>
BI++;<br>
}<br>
@@ -1486,7 +1495,7 @@ protected:<br>
static unsigned CountBundleInputs(ArrayRef<OperandBundleDef> Bundles) {<br>
unsigned Total = 0;<br>
for (auto &B : Bundles)<br>
- Total += B.Inputs.size();<br>
+ Total += B.input_size();<br>
return Total;<br>
}<br>
};<br>
<br>
Modified: llvm/trunk/lib/AsmParser/LLParser.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/LLParser.cpp?rev=253446&r1=253445&r2=253446&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/LLParser.cpp?rev=253446&r1=253445&r2=253446&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/AsmParser/LLParser.cpp (original)<br>
+++ llvm/trunk/lib/AsmParser/LLParser.cpp Wed Nov 18 02:30:07 2015<br>
@@ -1972,17 +1972,13 @@ bool LLParser::ParseOptionalOperandBundl<br>
if (ParseStringConstant(Tag))<br>
return true;<br>
<br>
- BundleList.emplace_back();<br>
- auto &OBI = BundleList.back();<br>
-<br>
- OBI.Tag = std::move(Tag);<br>
-<br>
if (ParseToken(lltok::lparen, "expected '(' in operand bundle"))<br>
return true;<br>
<br>
+ std::vector<Value *> Inputs;<br>
while (Lex.getKind() != lltok::rparen) {<br>
// If this isn't the first input, we need a comma.<br>
- if (!OBI.Inputs.empty() &&<br>
+ if (!Inputs.empty() &&<br>
ParseToken(lltok::comma, "expected ',' in input list"))<br>
return true;<br>
<br>
@@ -1990,9 +1986,11 @@ bool LLParser::ParseOptionalOperandBundl<br>
Value *Input = nullptr;<br>
if (ParseType(Ty) || ParseValue(Ty, Input, PFS))<br>
return true;<br>
- OBI.Inputs.push_back(Input);<br>
+ Inputs.push_back(Input);<br>
}<br>
<br>
+ BundleList.emplace_back(std::move(Tag), std::move(Inputs));<br>
+<br>
Lex.Lex(); // Lex the ')'.<br>
}<br>
<br>
<br>
Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=253446&r1=253445&r2=253446&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=253446&r1=253445&r2=253446&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)<br>
+++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Nov 18 02:30:07 2015<br>
@@ -5120,10 +5120,7 @@ std::error_code BitcodeReader::parseFunc<br>
if (Record.size() < 1 || Record[0] >= BundleTags.size())<br>
return error("Invalid record");<br>
<br>
- OperandBundles.emplace_back();<br>
- OperandBundles.back().Tag = BundleTags[Record[0]];<br>
-<br>
- std::vector<Value *> &Inputs = OperandBundles.back().Inputs;<br>
+ std::vector<Value *> Inputs;<br>
<br>
unsigned OpNum = 1;<br>
while (OpNum != Record.size()) {<br>
@@ -5133,6 +5130,7 @@ std::error_code BitcodeReader::parseFunc<br>
Inputs.push_back(Op);<br>
}<br>
<br>
+ OperandBundles.emplace_back(BundleTags[Record[0]], std::move(Inputs));<br>
continue;<br>
}<br>
}<br>
<br>
Modified: llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp?rev=253446&r1=253445&r2=253446&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp?rev=253446&r1=253445&r2=253446&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp Wed Nov 18 02:30:07 2015<br>
@@ -1195,7 +1195,7 @@ bool llvm::InlineFunction(CallSite CS, I<br>
MergedDeoptArgs.insert(MergedDeoptArgs.end(), ChildOB.Inputs.begin(),<br>
ChildOB.Inputs.end());<br>
<br>
- OpDefs.emplace_back("deopt", std::move(MergedDeoptArgs));<br>
+ OpDefs.emplace_back(StringRef("deopt"), std::move(MergedDeoptArgs));<br>
}<br>
<br>
Instruction *NewI = nullptr;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>