<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Dec 20, 2010, at 11:39 AM, dalej wrote</div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Dec 18, 2010, at 1:53 PM, Chris Lattner wrote:</div><blockquote type="cite"><div><blockquote type="cite">+++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Fri Dec 17 15:45:49 2010<br></blockquote><blockquote type="cite">@@ -3171,6 +3171,26 @@<br></blockquote><blockquote type="cite"> DAG.getConstant(c1 + c2, N1.getValueType()));<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ // fold (srl (trunc (srl x, c1)), c2) -> 0 or (trunc (srl x, (add c1, c2)))<br></blockquote><blockquote type="cite">+ // This is only valid if the OpSizeInBits + c1 = size of inner shift<br></blockquote><br>Please end the comment with a ".". Do we already do this for arithmetic shifts, and shift left (with extension instead of trunc) as well?<br></div></blockquote><div><br></div><div>It is not valid for arithmetic right shifts; the trunc is ANDing off sign bits, not zeroes. It is not done for shift left; I can add that. Although I'm not sure how it could arise from real code.</div><br><blockquote type="cite"><div><blockquote type="cite">+ if (N1C && N0.getOpcode() == ISD::TRUNCATE &&<br></blockquote><blockquote type="cite">+ N0.getOperand(0).getOpcode() == ISD::SRL &&<br></blockquote><blockquote type="cite">+ N0.getOperand(0)->getOperand(1).getOpcode() == ISD::Constant) {<br></blockquote><br>Please use isa<ConstantSDNode>(N0.getOperand(0)->getOperand(1)) which makes the cast<> more obviously right.<br></div></blockquote><div><br></div><div>OK, but this is following other code in the area.</div><br><blockquote type="cite"><div><blockquote type="cite">+ uint64_t c1 = <br></blockquote><blockquote type="cite">+ cast<ConstantSDNode>(N0.getOperand(0)->getOperand(1))->getZExtValue();<br></blockquote><blockquote type="cite">+ uint64_t c2 = N1C->getZExtValue();<br></blockquote><br>This can break if the value is > i64. Instead of doing this on getZExtValue(), I'd suggest doing the math on APInt's.<br></div></blockquote><div><br></div><div>These are shift counts, not the value being shifted. I think it's reasonable to assume number of bits per word is less than 18446744073709551616. There is lots of nearby code that does so.</div><br><blockquote type="cite"><div><blockquote type="cite">+ EVT InnerShiftVT = N0.getOperand(0)->getOperand(1).getValueType();<br></blockquote><blockquote type="cite">+ uint64_t InnerShiftSize = InnerShiftVT.getScalarType().getSizeInBits();<br></blockquote><blockquote type="cite">+ if (c1 + OpSizeInBits == InnerShiftSize) {<br></blockquote><blockquote type="cite">+ if (c1 + c2 >= InnerShiftSize)<br></blockquote><blockquote type="cite">+ return DAG.getConstant(0, VT);<br></blockquote><blockquote type="cite">+ return DAG.getNode(ISD::TRUNCATE, N0->getDebugLoc(), VT,<br></blockquote><br>Please add a short comment explaining what you're doing here. What is the "c1 + OpSizeInBits == InnerShiftSize" check doing?<br></div></blockquote><div><br></div><div>The comment without the period explains it. I'll move it down, I guess.</div><br><blockquote type="cite"><div><blockquote type="cite">+; RUN: llc < %s -mtriple=x86_64-apple-darwin | FileCheck %s<br></blockquote><blockquote type="cite">+; Formerly there were two shifts. 8771012.<br></blockquote><br>Please add a <a href="rdar://">rdar://</a> prefix so that grep will pick it up, thanks!<br></div></blockquote></div></div></blockquote></div><br><div>Ok, thanks, please do.</div><div><br></div><div>-Chris</div></body></html>