[PATCH] D15302: [Greedy regalloc] Replace analyzeSiblingValues with something new [Part1]

[Wei Mi via llvm-commits]

I am rebasing the patch now.

Wei.

On Mon, Mar 14, 2016 at 11:43 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> qcolombet added a comment.
>
> Hi Wei,
>
> Could you rebase your patches, they do not apply cleanly for me.
>
> Also, while I am here, a couple of inlined comments :).
>
> Cheers,
> -Quentin
>
>
> ================
> Comment at: include/llvm/CodeGen/LiveRangeEdit.h:148
> @@ -143,2 +147,3 @@
>    unsigned get(unsigned idx) const { return NewRegs[idx+FirstNew]; }
> +  void pop_back() { NewRegs.pop_back(); }
>
> ----------------
> This seems strange that the API allows to drop some of the new registers.
> At the very least, we should document (i.e., put explanatory comments) why this is useful and why it is okay to drop such references.
> In general, it should not.
>
> ================
> Comment at: include/llvm/CodeGen/LiveRangeEdit.h:184
> @@ +183,3 @@
> +    VNInfo *OrigVNI;        // ParentVNI.def may be a copy only. OrigVNI.def
> +                            // contains the real expr for remat.
> +    MachineInstr *OrigMI;   // Instruction defining OrigVNI.
> ----------------
> Instead of having at additional field which may be the same as ParentVNI in a lot of cases, this one could be computed.
> Then, if this has some performance problem, we can think of a better caching mechanism.
>
> ================
> Comment at: include/llvm/CodeGen/LiveRangeEdit.h:235
> @@ -220,3 +234,3 @@
>    /// as currently those new intervals are not guaranteed to spill.
> -  void eliminateDeadDefs(SmallVectorImpl<MachineInstr*> &Dead,
> -                         ArrayRef<unsigned> RegsBeingSpilled = None);
> +  /// NoSplit indicates it is used after the iterations of selectOrSplit and
> +  /// registers should not be split into new intervals.
> ----------------
> Replace 'it' by this live interval or something.
> The context is now high in the source file and repeating it wouldn't hurt IMO.
>
> ================
> Comment at: lib/CodeGen/InlineSpiller.cpp:56
> @@ -57,1 +55,3 @@
>  namespace {
> +class HoistSpiller {
> +  MachineFunction &MF;
> ----------------
> Since this class does not inherit from Spiller, what about naming it HoistSpillHelper or something.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D15302
>
>
>