<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Looks great. IMO it’s always better to over-document these kinds of fiddly transforms. If they’re wrong somehow, they can be a real pain to track down, so having a good explanation of the intended functionality is incredibly useful.<div><br></div><div>—Owen</div><div><br><div><div>On Jan 8, 2014, at 7:52 AM, Richard Sandiford <<a href="mailto:rsandifo@linux.vnet.ibm.com">rsandifo@linux.vnet.ibm.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Owen Anderson <<a href="mailto:resistor@mac.com">resistor@mac.com</a>> writes:<br><blockquote type="cite">Patch 1 LGTM.<br></blockquote><br>r198768, thanks.<br><br><blockquote type="cite">Patch 2:<br><br><blockquote type="cite">+// Check whether Neg == OpSize - Pos.<br></blockquote><br>Please add a more explanatory function comment.<br><br><blockquote type="cite">+ // Check for cases where Pos = NegOp1 + PosC and where:<br>+ // Neg = OpSize - Pos<br>+ // = OpSize - (NegOp1 + PosC)<br>+ // = (OpSize - PosC) - NegOp1<br>+ // NegC = OpSize - PosC<br>+ // OpSize = NegC + PosC<br></blockquote><br>I think this comment could do with an s-expression-style example of the<br>overall pattern you’re trying to match.<br><br>Patch 3:<br><br><blockquote type="cite">+ // Remove a mask of OpSize - 1.<br>+ unsigned LoBits = 0;<br>+ if (Neg.getOpcode() == ISD::AND &&<br>+ isPowerOf2_64(OpSize) &&<br>+ Neg.getOperand(1).getOpcode() == ISD::Constant &&<br>+ cast<ConstantSDNode>(Neg.getOperand(1))->getAPIntValue() == OpSize -<br>1) {<br>+ Neg = Neg.getOperand(0);<br>+ LoBits = Log2_64(OpSize);<br>+ }<br>+<br></blockquote><br>Again, please explain the logic here.<br><br><blockquote type="cite">+ if (LoBits)<br>+ return Width.getLoBits(LoBits) == 0;<br>+ return Width == OpSize;<br></blockquote><br>ENOCOMMENT<br></blockquote><br>OK, here's patches 2 and 3 with extra (and in a couple of cases, fixed)<br>commentary. Hope this isn't going too far the other way.<br><br>Thanks,<br>Richard<br><br><span><rotate-2.diff></span><span><rotate-3.diff></span></div></blockquote></div><br></div></body></html>