[PATCH] D30780: [APInt] Add APInt::insertBits() method to insert an APInt into a larger APInt

[Filipe Cabecinhas via llvm-commits]

On Thu, 9 Mar 2017 at 22:19, Hans Wennborg via Phabricator <
reviews at reviews.llvm.org> wrote:

> hans added inline comments.
>
>
> ================
> Comment at: lib/Support/APInt.cpp:592
> +void APInt::insertBits(const APInt &subBits, unsigned bitPosition) {
> +  unsigned subBitWidth = subBits.getBitWidth();
> +  assert((subBitWidth + bitPosition) <= BitWidth && "Illegal bit
> insertion");
> ----------------
> I suppose subBitWidth can't be zero, but maybe assert that? Otherwise we'd
> run into trouble with the shifts and so on.
>
>
> ================
> Comment at: lib/Support/APInt.cpp:597
> +  if (subBitWidth == BitWidth) {
> +    *this = subBits;
> +    return;
> ----------------
> I would assert bitPosition == 0 here, just to make it easier for the reader

SubBitWidth + bitPosition has been asserted as being <= BitWidth, so I
wouldn't say that assert is necessary, but ok for clarity. It's not like
the code will be much different :-)


>
>
> ================
> Comment at: lib/Support/APInt.cpp:617
> +    pVal[loWord] &= ~(mask << loBit);
> +    pVal[loWord] |= (subBits.VAL << loBit);
> +    return;
> ----------------
> This code is the same as for the isSingleWord() case except for
> pVal[loWord] instead of VAL.
>
> If you really wanted to unify the two, we could use `getRawData()[loWord]
> & =` instead.
>
> Not sure if it's worth it though.

nice suggestion, but I fear it might become harder to read, no?


>
>
> ================
> Comment at: lib/Support/APInt.cpp:638
> +
> +  // General case - set/clear individual bits in dst based on src.
> +  for (unsigned i = 0; i != subBitWidth; ++i) {
> ----------------
> It seems we could do better. Can't we do something similar to
> `setBitsSlowCase`, so we process word-by-word even when bitPosition isn't
> aligned?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D30780
>
>

Re: Simon

Nice idea, to use extractBits for that test.

Thank you,

 Filipe

>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170310/2d5e8849/attachment.html>