<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-05-29 23:08 GMT-07:00 Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Marcello<div><br></div><div>This is great. Thanks for working on this.</div><div>
<br></div><div>So the first patch "flag_nodes.patch” contains quite a bit of code cleanup. Its all good cleanup, but as there’s quite a lot of it can you please separate it out and rebase the other patches on top of the cleanup work.</div>
<div><div><br></div><div>The * here is a neat trick, but not really valid for a bool. Would && work the same?</div><div><br></div><div><div>+ SubclassData = </div><div>+ (SubclassData & ~NUW) | (b * NUW);</div>
</div><div><br></div></div></div></blockquote><div><br></div><div>Hmm, I have to admit I copied this from the approach used in the Operator.h:89 for the "OverflowingBinaryOperator" class.</div><div><br></div><div>
I thought it was strange, but I assumed it worked because it was there. Maybe would be a good idea to change it everywhere? (in two separate patches)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div></div><div>Inside</div><div> <br></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div></div><div>+SDValue SelectionDAG::getBinaryNode(unsigned Opcode, SDLoc DL, EVT VT,<br>+ SDValue N1, SDValue N2, bool nuw, bool nsw,<br>+ bool exact)</div>
<div><br></div><div>I think you need to call (and add appropriate code to) AddNodeIDNodeCustom instead of AddNodeIDNode to handle generating the correct hash for subclass data. See how ISD::LOAD is handled in that method. A test case to ensure that the same binary operators with different flags aren’t CSEd would be useful to have for this.</div>
<div><br></div></div></div></blockquote><div><br></div><div>I see :) I'll do it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div></div><div><div>+ const BinaryOperator *BinOp = dyn_cast<const BinaryOperator>(&I);</div><div>+ // If it is a BinaryOperator use getBinaryNode to create the node</div><div>+ if (BinOp != nullptr) {</div>
<div>+ const bool IsOverflowingBin = isa<OverflowingBinaryOperator>(BinOp);</div><div>+ const bool IsExactOp = isa<PossiblyExactOperator>(BinOp);</div><div>+ const bool nuw = IsOverflowingBin ? BinOp->hasNoUnsignedWrap() : false;</div>
<div>+ const bool nsw = IsOverflowingBin ? BinOp->hasNoSignedWrap() : false;</div><div>+ const bool exact = IsExactOp ? BinOp->isExact() : false;</div><div>+ SDValue BinNodeValue = DAG.getBinaryNode(</div><div>
+ OpCode, getCurSDLoc(), Op1.getValueType(), Op1, Op2, nuw, nsw, exact);</div><div>+ setValue(&I, BinNodeValue);</div><div>+ } else {</div><div>+ setValue(&I,</div><div>+ DAG.getNode(OpCode, getCurSDLoc(), Op1.getValueType(), Op1, Op2));</div>
<div>+ }</div></div><div><br></div><div>You don’t need the ‘!= nullptr’. And personally i’d move some of 'OpCode, getCurSDLoc(), …’ up to the same line as DAG.getBinaryNode to make it read easier. You can also just return after the first setValue and avoid the else branch entirely.</div>
<div><br></div></div></div></blockquote><div><br></div><div>You are right. I originally had tests failing because what came down wasn't a BinaryOperator (ConstantDataVector for example), but actually casting to the more specific PossiblyExactOperator or OverflowingBinaryOperator makes it work without the if! Didn't notice they had the "hasNoSignedWrap/isExact" methods too (well, it kind of makes sense though ... :P)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div></div><div>In “x86_opt_wrap.patch”, you have some tabbing which has gone bad, but otherwise the content is good. Its a shame to see a static_cast not a more LLVM appropriate cast<> or dyn_cast but thats because BinarySDNode doesn’t implement classof and not something that should block your patch right now. Feel free to fix that later if you want to.</div>
<div><br></div><div>I’d fold the test patch in to the opt patch just so that they are committed together. The test case could do with being reduced to the minimal possible test to demonstrate this patch (probably just the first BB), but otherwise is good.</div>
<div><br></div></div></div></blockquote><div><br></div><div>Ok, will do!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div></div><div>Thanks,</div><div>Pete</div><div><br></div><div><div class=""><div>On May 29, 2014, at 7:44 PM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>> wrote:</div>
<br></div><blockquote type="cite"><div><div class="h5"><div dir="ltr"><span style="font-family:arial,sans-serif;font-size:13px">Hello,</span><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
here is a proposed patch to pass down the BinaryOperator's "NoUnsignedWrap/NoSignedWrap and IsExact" flags down to the SDNodes so that backends can use them for additional optimizations.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">The core of the patch is using the SubclassData field of SDNode for BinarySDNodes to store the flags information.</div>
<div style="font-family:arial,sans-serif;font-size:13px">SelectionDAGBuilder will then set the correct flag information the moment the Binary instruction is visited during construction.</div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px">In doing this I also refactored a little bit the getNode() method accepting two operands, extracting the main constant folding logic into a separate static function.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">The patch implementing this is the file "flag_nodes.patch" attached.</div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px">In addition I implemented an optimization where an unneeded "test instruction" that was emitted from the X86 backend is not emitted anymore. This optimization uses this new feature.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">This optimization is contained in the "x86_opt_wrap.patch" attached.</div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px">I also added a test case that exercises the optimization in the "factorial_test.patch"</div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px">The patch makes this snippet of code:</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
LBB0_1: ## %while.body<br></div><div style="font-family:arial,sans-serif;font-size:13px"><div> ## =>This Inner Loop Header: Depth=1</div><div>
imulq %rdi, %rax</div><div> decq %rdi</div><div> testq %rdi, %rdi</div><div> jg LBB0_1</div></div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">become this:</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
<div><br></div><div><br></div><div>LBB0_1: ## %while.body<br></div><div><div> ## =>This Inner Loop Header: Depth=1</div><div> imulq %rdi, %rax</div>
<div> decq %rdi</div><div> jg LBB0_1</div></div></div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">if the NSW flag is set on the SDNode that represents the DEC instruction (presumably an ADD or a SUB node in this case).</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">If somebody could review this it would be awesome :)</div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px">Cheers,</div><div style="font-family:arial,sans-serif;font-size:13px">Marcello</div></div>
</div></div><div class=""><span><factorial_test.patch></span><span><flag_nodes.patch></span><span><x86_opt_wrap.patch></span>_______________________________________________<br>llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></blockquote></div><br></div></div></blockquote></div><br></div></div>