<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>