[llvm-commits] [PATCH] PERF: Reuse data fragments while lowering

[David Meyer]

Rafael,

New patch attached. Also, visual diff at:

http://codereview.chromium.org/5119002/


> +  /// \brief Update the layout to coalesce Src into Dst.
> +  void CoalesceFragments(MCFragment *Src, MCFragment *Dst);
>
> Add a note that the actual contents are not copied.
>

Done.


>
> +  if (isFragmentUpToDate(Src)) {
> +    if (LastValidFragment == Src)
> +      LastValidFragment = Dst;
> +    Dst->EffectiveSize += Src->EffectiveSize;
> +  } else {
> +    // We don't know the effective size of Src, so we have to invalidate
> Dst.
> +    UpdateForSlide(Dst, 0);
> +  }
>
> You have to invalidate Dst in any case, no? Adding something to the
> fragment will cause the addresses in fragment following it to change.
> This probably works in the current case since we are scanning the
> fragments in order.
>
> Is it too expensive to just call UpdateForSlide for both cases?
>

Most of the time, nothing ever gets invalidated.

When two fragments are coalesced, they are combined into one fragment whose
total size is equal to the sum of the sizes of the original two. So the
addresses of later fragments won't change. Nothing needs to be invalidated
unless Dst happens to be the LastValidFragment. (in which case, only Dst is
invalidated).


>
> +      // Since we may have removed fragments, fix the layout order.
> +      it2->setLayoutOrder(FragmentIndex++)
>
> merged would probably be a better description.
>

Done.


>
> +        it2 = CurDF;
>
> Setting an iterator in this for loop is a bit strange. Would you mind
> removing the ++it2 so that we have a
>
> if (...)
>  it2 = CurDF;
> else
>  ++it2;
>
> That way all the iterator updates are close to each other.
>

I did not change the way this is done, only the name of the variables. The
iterator needs to be repositioned since we deleted the instruction fragment
it was pointing to. I found the original way to be very clear.

Note that the ++it2 needs to happen in addition to the it2 = CurDF.
(The next fragment to look at is CurDF+1)


>
> > - David Meyer
>
> Thanks a lot for working on this. Hopefully it will close some of the
> performance gap of "llc -filetype=obj" X "llc && llvm-mc".
>

I ran my own test on a 1,000,000 line .s file ... there was too much random
variance to measure a statistically significant difference. The assembler is
already so fast... the greatest advantage of this patch may be reduced
memory usage.

Thanks,
 - David Meyer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101118/c2ed2d78/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: instlowering-2.patch
Type: application/octet-stream
Size: 5783 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101118/c2ed2d78/attachment.obj>