[PATCH] Use Rvalue refs in APInt

Sat Jun 4 09:10:04 PDT 2016

Looks like you just have a variable rename in the unary minus
implementation - commit separately or drop that change perhaps?

I don't think you need versions that take two rvalue refs (+(&&, &&)), is
there? (until +=/-= get smart and have an overload that takes an rvalue ref
parameter and uses it to steal the right hand side buffer if it's bigger
than the left hand side buffer or something like that?)

& can you pass by value instead of by rvalue ref - does that work/choose
the right overloads?

inline APInt operator-(APInt RHS, const APInt &LHS) {
  RHS += LHS;
  return RHS; // shouldn't need std::move here because you're returning a
local
}

Then you shouldn't need the op-(const APInt&,const APInt&) version, for
example.

Tests?

On Fri, Jun 3, 2016 at 10:42 AM, Pete Cooper <peter_cooper at apple.com> wrote:

> Hi David, Sanjoy
>
> Here’s an updated patch which provides all of the combinations of
> operator[+-] I can think to add.
>
> All of the new ones are outside the class definition so that we can reduce
> duplication and have them call each other.
>
> The one thing I noticed while doing this work was that the already
> existing operator+= and -= methods really did exactly what I wanted.  So
> i’ve implemented + and - in terms of += and -=.
>
> Is that ok, or is it frowned upon?  I can imagine some people would prefer
> that += calls + and not the other way round.  But it is very convenient as
> you can see with this patch.
>
> Comments very welcome.
>
> BTW, this reduces total allocations by about 400k from 19.1M to 18.7M.
>
> Cheers,
> Pete
>
>
>
> On Jun 2, 2016, at 3:24 PM, Pete Cooper via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>
> On Jun 2, 2016, at 2:28 PM, Sanjoy Das <sanjoy at playingwithpointers.com>
> wrote:
>
> On Wed, Jun 1, 2016 at 9:43 AM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
> Another interesting data point is the compile time.  On my test case,
> SCEV::getRange is 8.9% of compile time which is a lot.  But of that, 6.3%
> is just in ConstantRange::multiply.  This method is heavy APInt code, and
> especially malloc traffic.
>
>
> Yeah, that is definitely too high! Just to check: I assume you mean
> 8.9% of opt -O2 or something similar?
>
> Yep, thats right.  ‘opt -O2 verify-uselistorder.bc -o opt.bc’.  The
> verify-uselistorder is the pre optimized, but post linked, bitcode when
> LTOing that tool.
>
> BTW, I just looked at the latest numbers and the commits i’ve made so far
> save 3% of compile time on this use case.  So the 8.9% is more like 5.9%
> now.  And still a little more to come.
>
>
> Many of the speedup’s i’ve been finding involve doing less work (r271020
> which avoids the latter half of ConstantRange::multiply and saves 3M
> allocations), and fixing cases of unnecessary APInt allocations (r270959).
> This patch is along the same lines as the latter where we have malloc
> traffic we can avoid.
>
>
> Making too many fixes on the APInt algorithms to avoid allocations
> seems like we're solving the issue at the wrong layer.  I think fixing
> infrastructural issues so that we _can_ be a little sloppy (within
> reason) in extending integers without thinking too much about malloc
> traffic is the right path.
>
> I completely agree.  There are certainly limits to how far to push this.
> For example, this code in ConstantRange::multiply:
>
>   auto L = {this_min * Other_min, this_min * Other_max,
>             this_max * Other_min, this_max * Other_max};
>
>
> Once I have the Rvalue ref version of the APInt methods (a change which I
> think is reasonable), the above could be changed to:
>
>   auto L = {this_min * Other_min, std::move(this_min) * Other_max,
>             this_max * std::move(Other_min), std::move(this_max) *
> Other_max};
>
> This would avoid 3 allocations out of 4 because we will then use the
> Rvalue APInt methods.  However, I think this might
> be a little too much hacking.  So yeah, I totally agree with you, and
> hopefully we can solve cases like this one in a more
> reasonable way than gratuitous use of std::move() or other APInt hackery :)
>
>
> But you're doing the work, so you get to decide the path forward. :)
>
> Sounds good to me :)
>
>
>
> ConstantRange stats (bit width and count of hits in
> ConstantRange::ConstantRange)
>
>
> This is great!  Is this a bootstrap of clang or something?
>
> Actually same use case as before.  ‘opt -O2 verify-uselistorder’.  Its a
> nice small bit code which takes about 20s to optimize.
>
>
> Btw, there are couple of bitwidths here that I find interesting, e.g.
> I'd not have expected this many i70 ConstantRange allocations.
>
> Yeah, some of these are a bit surprising.  2^n and (2^n)+1 both seem
> likely due to the IR itself and SCEV, but anything else is a little odd.  I
> may take a look at the 258 bit case just because there are so many of them.
>
> Pete
>
>
> -- Sanjoy
>
> 1: 30850028
> 2: 7238
> 3: 5733
> 4: 92
> 5: 817
> 6: 294
> 7: 192
> 8: 363498
> 9: 896
> 11: 330
> 12: 378
> 13: 385
> 14: 125
> 16: 30256
> 18: 272
> 20: 98
> 24: 10
> 25: 62
> 26: 13
> 27: 181
> 28: 8
> 31: 98
> 32: 2003134
> 33: 132
> 34: 128
> 36: 76
> 38: 2130
> 41: 3
> 57: 262
> 58: 244
> 59: 342
> 60: 2418
> 61: 1211
> 62: 190
> 63: 226
> 64: 5118228
> 65: 128400
> 66: 4236
> 67: 14826
> 68: 15408
> 69: 13417
> 70: 7959
> 71: 347
> 96: 88
> 128: 364826
> 129: 379580
> 130: 19092
> 256: 4734
> 257: 19132
> 258: 71826
> 514: 4650
>
>
>
>
> --
> Sanjoy Das
> http://playingwithpointers.com
>
>
> _______________________________________________
> 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/20160604/b5f02b1d/attachment.html>