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