<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 22, 2016 at 10:53 AM, Justin Lebar <span dir="ltr"><<a href="mailto:jlebar@google.com" target="_blank">jlebar@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Passing const ref to a unique_ptr is generally not done.<br>
<br>
</span>Indeed; normally I'd pass a (possibly const) raw pointer. But in this<br>
case, the function in question is used as a functor param to<br>
std::sort. So it can't take raw pointers, as there's no implicit<br>
conversion from unique_ptr to a raw pointer.<br></blockquote><div><br>Ah, sorry - that makes sense. I hadn't looked at any context there. (I'd usually write a predicate as a lambda in the point-of-call, unless it's used in a few places - perhaps that's the case here)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Perhaps these should be passed by non-const ref instead<br>
<br>
</span>This seems worse than passing by const-ref? This would be the moral<br>
equivalent of passing by non-const double-pointer, when I want the<br>
moral equivalent of passing by const single-pointer.<br></blockquote><div><br></div><div>Sorry, I meant const T& instead of const unique_ptr<T> & - but, as you said, not applicable since it needs to be used in std::sort over a container of unique_ptr.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
On Mon, Feb 22, 2016 at 10:49 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Feb 22, 2016 at 9:51 AM, Justin Lebar via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: jlebar<br>
>> Date: Mon Feb 22 11:51:28 2016<br>
>> New Revision: 261541<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=261541&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=261541&view=rev</a><br>
>> Log:<br>
>> [ifcnv] Use unique_ptr in IfConversion. NFC<br>
>><br>
>> Reviewers: rnk<br>
>><br>
>> Subscribers: llvm-commits<br>
>><br>
>> Differential Revision: <a href="http://reviews.llvm.org/D17466" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17466</a><br>
>><br>
>> Modified:<br>
>> llvm/trunk/lib/CodeGen/IfConversion.cpp<br>
>><br>
>> Modified: llvm/trunk/lib/CodeGen/IfConversion.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/IfConversion.cpp?rev=261541&r1=261540&r2=261541&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/IfConversion.cpp?rev=261541&r1=261540&r2=261541&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/CodeGen/IfConversion.cpp (original)<br>
>> +++ llvm/trunk/lib/CodeGen/IfConversion.cpp Mon Feb 22 11:51:28 2016<br>
>> @@ -198,10 +198,12 @@ namespace {<br>
>> bool ValidDiamond(BBInfo &TrueBBI, BBInfo &FalseBBI,<br>
>> unsigned &Dups1, unsigned &Dups2) const;<br>
>> void ScanInstructions(BBInfo &BBI);<br>
>> - void AnalyzeBlock(MachineBasicBlock *MBB, std::vector<IfcvtToken*><br>
>> &Tokens);<br>
>> + void AnalyzeBlock(MachineBasicBlock *MBB,<br>
>> + std::vector<std::unique_ptr<IfcvtToken>> &Tokens);<br>
>> bool FeasibilityAnalysis(BBInfo &BBI, SmallVectorImpl<MachineOperand><br>
>> &Cond,<br>
>> bool isTriangle = false, bool RevBranch =<br>
>> false);<br>
>> - void AnalyzeBlocks(MachineFunction &MF, std::vector<IfcvtToken*><br>
>> &Tokens);<br>
>> + void AnalyzeBlocks(MachineFunction &MF,<br>
>> + std::vector<std::unique_ptr<IfcvtToken>> &Tokens);<br>
>> void InvalidatePreds(MachineBasicBlock *BB);<br>
>> void RemoveExtraEdges(BBInfo &BBI);<br>
>> bool IfConvertSimple(BBInfo &BBI, IfcvtKind Kind);<br>
>> @@ -240,7 +242,8 @@ namespace {<br>
>> }<br>
>><br>
>> // IfcvtTokenCmp - Used to sort if-conversion candidates.<br>
>> - static bool IfcvtTokenCmp(IfcvtToken *C1, IfcvtToken *C2) {<br>
>> + static bool IfcvtTokenCmp(const std::unique_ptr<IfcvtToken> &C1,<br>
>> + const std::unique_ptr<IfcvtToken> &C2) {<br>
><br>
><br>
> Passing const ref to a unique_ptr is generally not done. It's an unnecessary<br>
> requirement (since you can't take ownership from it inside the function, why<br>
> place a restriction on how your caller is managing the memory of the<br>
> object?). Perhaps these should be passed by non-const ref instead?<br>
><br>
>><br>
>> int Incr1 = (C1->Kind == ICDiamond)<br>
>> ? -(int)(C1->NumDups + C1->NumDups2) : (int)C1->NumDups;<br>
>> int Incr2 = (C2->Kind == ICDiamond)<br>
>> @@ -309,7 +312,7 @@ bool IfConverter::runOnMachineFunction(M<br>
>> MF.RenumberBlocks();<br>
>> BBAnalysis.resize(MF.getNumBlockIDs());<br>
>><br>
>> - std::vector<IfcvtToken*> Tokens;<br>
>> + std::vector<std::unique_ptr<IfcvtToken>> Tokens;<br>
>> MadeChange = false;<br>
>> unsigned NumIfCvts = NumSimple + NumSimpleFalse + NumTriangle +<br>
>> NumTriangleRev + NumTriangleFalse + NumTriangleFRev + NumDiamonds;<br>
>> @@ -319,15 +322,13 @@ bool IfConverter::runOnMachineFunction(M<br>
>> bool Change = false;<br>
>> AnalyzeBlocks(MF, Tokens);<br>
>> while (!Tokens.empty()) {<br>
>> - IfcvtToken *Token = Tokens.back();<br>
>> + std::unique_ptr<IfcvtToken> Token = std::move(Tokens.back());<br>
>> Tokens.pop_back();<br>
>> BBInfo &BBI = Token->BBI;<br>
>> IfcvtKind Kind = Token->Kind;<br>
>> unsigned NumDups = Token->NumDups;<br>
>> unsigned NumDups2 = Token->NumDups2;<br>
>><br>
>> - delete Token;<br>
>> -<br>
>> // If the block has been evicted out of the queue or it has already<br>
>> been<br>
>> // marked dead (due to it being predicated), then skip it.<br>
>> if (BBI.IsDone)<br>
>> @@ -414,13 +415,6 @@ bool IfConverter::runOnMachineFunction(M<br>
>> MadeChange |= Change;<br>
>> }<br>
>><br>
>> - // Delete tokens in case of early exit.<br>
>> - while (!Tokens.empty()) {<br>
>> - IfcvtToken *Token = Tokens.back();<br>
>> - Tokens.pop_back();<br>
>> - delete Token;<br>
>> - }<br>
>> -<br>
>> Tokens.clear();<br>
>> BBAnalysis.clear();<br>
>><br>
>> @@ -768,8 +762,8 @@ bool IfConverter::FeasibilityAnalysis(BB<br>
>> /// AnalyzeBlock - Analyze the structure of the sub-CFG starting from<br>
>> /// the specified block. Record its successors and whether it looks like<br>
>> an<br>
>> /// if-conversion candidate.<br>
>> -void IfConverter::AnalyzeBlock(MachineBasicBlock *MBB,<br>
>> - std::vector<IfcvtToken*> &Tokens) {<br>
>> +void IfConverter::AnalyzeBlock(<br>
>> + MachineBasicBlock *MBB, std::vector<std::unique_ptr<IfcvtToken>><br>
>> &Tokens) {<br>
>> struct BBState {<br>
>> BBState(MachineBasicBlock *BB) : MBB(BB), SuccsAnalyzed(false) {}<br>
>> MachineBasicBlock *MBB;<br>
>> @@ -867,8 +861,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>> // \ /<br>
>> // TailBB<br>
>> // Note TailBB can be empty.<br>
>> - Tokens.push_back(new IfcvtToken(BBI, ICDiamond, TNeedSub|FNeedSub,<br>
>> Dups,<br>
>> - Dups2));<br>
>> + Tokens.push_back(llvm::make_unique<IfcvtToken>(<br>
>> + BBI, ICDiamond, TNeedSub | FNeedSub, Dups, Dups2));<br>
>> Enqueued = true;<br>
>> }<br>
>><br>
>> @@ -883,7 +877,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>> // | TBB<br>
>> // | /<br>
>> // FBB<br>
>> - Tokens.push_back(new IfcvtToken(BBI, ICTriangle, TNeedSub, Dups));<br>
>> + Tokens.push_back(<br>
>> + llvm::make_unique<IfcvtToken>(BBI, ICTriangle, TNeedSub,<br>
>> Dups));<br>
>> Enqueued = true;<br>
>> }<br>
>><br>
>> @@ -891,7 +886,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>> MeetIfcvtSizeLimit(*TrueBBI.BB, TrueBBI.NonPredSize +<br>
>> TrueBBI.ExtraCost,<br>
>> TrueBBI.ExtraCost2, Prediction) &&<br>
>> FeasibilityAnalysis(TrueBBI, BBI.BrCond, true, true)) {<br>
>> - Tokens.push_back(new IfcvtToken(BBI, ICTriangleRev, TNeedSub,<br>
>> Dups));<br>
>> + Tokens.push_back(<br>
>> + llvm::make_unique<IfcvtToken>(BBI, ICTriangleRev, TNeedSub,<br>
>> Dups));<br>
>> Enqueued = true;<br>
>> }<br>
>><br>
>> @@ -906,7 +902,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>> // | TBB---> exit<br>
>> // |<br>
>> // FBB<br>
>> - Tokens.push_back(new IfcvtToken(BBI, ICSimple, TNeedSub, Dups));<br>
>> + Tokens.push_back(<br>
>> + llvm::make_unique<IfcvtToken>(BBI, ICSimple, TNeedSub, Dups));<br>
>> Enqueued = true;<br>
>> }<br>
>><br>
>> @@ -918,7 +915,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>> FalseBBI.NonPredSize + FalseBBI.ExtraCost,<br>
>> FalseBBI.ExtraCost2, Prediction.getCompl())<br>
>> &&<br>
>> FeasibilityAnalysis(FalseBBI, RevCond, true)) {<br>
>> - Tokens.push_back(new IfcvtToken(BBI, ICTriangleFalse, FNeedSub,<br>
>> Dups));<br>
>> + Tokens.push_back(llvm::make_unique<IfcvtToken>(BBI,<br>
>> ICTriangleFalse,<br>
>> + FNeedSub, Dups));<br>
>> Enqueued = true;<br>
>> }<br>
>><br>
>> @@ -928,7 +926,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>> FalseBBI.NonPredSize + FalseBBI.ExtraCost,<br>
>> FalseBBI.ExtraCost2, Prediction.getCompl()) &&<br>
>> FeasibilityAnalysis(FalseBBI, RevCond, true, true)) {<br>
>> - Tokens.push_back(new IfcvtToken(BBI, ICTriangleFRev, FNeedSub,<br>
>> Dups));<br>
>> + Tokens.push_back(<br>
>> + llvm::make_unique<IfcvtToken>(BBI, ICTriangleFRev, FNeedSub,<br>
>> Dups));<br>
>> Enqueued = true;<br>
>> }<br>
>><br>
>> @@ -937,7 +936,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>> FalseBBI.NonPredSize + FalseBBI.ExtraCost,<br>
>> FalseBBI.ExtraCost2, Prediction.getCompl())<br>
>> &&<br>
>> FeasibilityAnalysis(FalseBBI, RevCond)) {<br>
>> - Tokens.push_back(new IfcvtToken(BBI, ICSimpleFalse, FNeedSub,<br>
>> Dups));<br>
>> + Tokens.push_back(<br>
>> + llvm::make_unique<IfcvtToken>(BBI, ICSimpleFalse, FNeedSub,<br>
>> Dups));<br>
>> Enqueued = true;<br>
>> }<br>
>> }<br>
>> @@ -951,8 +951,8 @@ void IfConverter::AnalyzeBlock(MachineBa<br>
>><br>
>> /// AnalyzeBlocks - Analyze all blocks and find entries for all<br>
>> if-conversion<br>
>> /// candidates.<br>
>> -void IfConverter::AnalyzeBlocks(MachineFunction &MF,<br>
>> - std::vector<IfcvtToken*> &Tokens) {<br>
>> +void IfConverter::AnalyzeBlocks(<br>
>> + MachineFunction &MF, std::vector<std::unique_ptr<IfcvtToken>><br>
>> &Tokens) {<br>
>> for (auto &BB : MF)<br>
>> AnalyzeBlock(&BB, Tokens);<br>
>><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>
><br>
><br>
</div></div></blockquote></div><br></div></div>