<div><br><div class="gmail_quote"><div>On Thu, 9 Mar 2017 at 22:19, Hans Wennborg via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">hans added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Support/APInt.cpp:592<br class="gmail_msg">
+void APInt::insertBits(const APInt &subBits, unsigned bitPosition) {<br class="gmail_msg">
+  unsigned subBitWidth = subBits.getBitWidth();<br class="gmail_msg">
+  assert((subBitWidth + bitPosition) <= BitWidth && "Illegal bit insertion");<br class="gmail_msg">
----------------<br class="gmail_msg">
I suppose subBitWidth can't be zero, but maybe assert that? Otherwise we'd run into trouble with the shifts and so on.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Support/APInt.cpp:597<br class="gmail_msg">
+  if (subBitWidth == BitWidth) {<br class="gmail_msg">
+    *this = subBits;<br class="gmail_msg">
+    return;<br class="gmail_msg">
----------------<br class="gmail_msg">
I would assert bitPosition == 0 here, just to make it easier for the reader</blockquote><div>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 :-)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Support/APInt.cpp:617<br class="gmail_msg">
+    pVal[loWord] &= ~(mask << loBit);<br class="gmail_msg">
+    pVal[loWord] |= (subBits.VAL << loBit);<br class="gmail_msg">
+    return;<br class="gmail_msg">
----------------<br class="gmail_msg">
This code is the same as for the isSingleWord() case except for pVal[loWord] instead of VAL.<br class="gmail_msg">
<br class="gmail_msg">
If you really wanted to unify the two, we could use `getRawData()[loWord] & =` instead.<br class="gmail_msg">
<br class="gmail_msg">
Not sure if it's worth it though.</blockquote><div>nice suggestion, but I fear it might become harder to read, no?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Support/APInt.cpp:638<br class="gmail_msg">
+<br class="gmail_msg">
+  // General case - set/clear individual bits in dst based on src.<br class="gmail_msg">
+  for (unsigned i = 0; i != subBitWidth; ++i) {<br class="gmail_msg">
----------------<br class="gmail_msg">
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?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
Repository:<br class="gmail_msg">
  rL LLVM<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D30780" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D30780</a><br class="gmail_msg">
<br class="gmail_msg">
</blockquote><div><br></div><div><br></div><div>Re: Simon</div><div><br></div><div>Nice idea, to use extractBits for that test.</div><div><br></div><div>Thank you,</div><div><br></div><div> Filipe</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>