[PATCH] D13185: Scheduler / Regalloc: use unique_ptr[] instead of std::vector

[David Blaikie via llvm-commits]

On Fri, Sep 25, 2015 at 5:42 PM, Matthias Braun via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> MatzeB added a comment.
>
> Given that the vectors are fixed size I don't see why we would need the
> additional complexity of std::vector (it has to manage the size and
> capacity, ...).


I'm often on the fence about this - in this case it seems like at least the
size is known/shared elsewhere.

I'd probably still have used std::dynarray, if it existed, which it doesn't
- and since the standards committee decided it wasn't worth having, I tend
to tip towards std::vector, rather than towards a raw array. But that's
just me.


> To me this change looks good with the nitpicks below addressed.
>
> Of course it's bad that llvm doesn't get the vector initialisation
> optimized, but this is a separate issue, would you agree David?
>

Sure, to a degree - maybe SmallVector would get the right benefits/have
better resize performance & or if it's a sufficiently obscure standard
library implementation that it's not worth optimizing for (perhaps
someone's fixed it/it's an old version/etc).

But I'm just rambling - certainly not going to stop the patch going in,
happy to leave it to other people to decide.


>
>
> ================
> Comment at: lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp:1221
> @@ -1220,3 +1220,3 @@
>  static void CheckForLiveRegDef(SUnit *SU, unsigned Reg,
> -                               std::vector<SUnit*> &LiveRegDefs,
> +                               std::unique_ptr<SUnit*[]> &LiveRegDefs,
>                                 SmallSet<unsigned, 4> &RegAdded,
> ----------------
> This can be a simple SUnit * now.
>
> ================
> Comment at: lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp:1243-1246
> @@ -1242,5 +1242,6 @@
>  static void CheckForLiveRegDefMasked(SUnit *SU, const uint32_t *RegMask,
> -                                     std::vector<SUnit*> &LiveRegDefs,
> +                                     std::unique_ptr<SUnit*[]>
> &LiveRegDefs,
>                                       SmallSet<unsigned, 4> &RegAdded,
> -                                     SmallVectorImpl<unsigned> &LRegs) {
> +                                     SmallVectorImpl<unsigned> &LRegs,
> +                                     unsigned NumRegs) {
>    // Look at all live registers. Skip Reg0 and the special CallResource.
> ----------------
> This could use an ArrayRef<SUnit*> instead of std::unique_ptr reference +
> NumRegs.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D13185
>
>
>
> _______________________________________________
> 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/20150925/76d35e28/attachment.html>