<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Daniel, </div><div><br></div><div>Thanks for the review!</div><div><br></div><div>+ // The SelectionDAG vectorizer can only handle pairs (trees of height = 2).<br><br></div><div>r187316.<br></div><div><br></div><div>-Nadav</div><div><br></div><div><div>On Jul 27, 2013, at 10:48 AM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: Helvetica; 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;"><div dir="ltr">Hi Nadav,<div><br></div><div>Okay.</div><div><br></div><div>1. The comment doesn't make this clear. I would suggest, at a minimum, updating it to mention pairs specifically, to avoid the issue in #2</div><div><br></div><div>2. If the day comes when the selectiondag store vectorizer handles more than pairs, and does so better, is anyone really going to remember this random 3 exists in the other vectorizer?</div><div><br></div><div>I would posit, based on experience, the answer is "no" :)</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jul 26, 2013 at 7:56 PM, Nadav Rotem<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div dir="auto"><div>Hi Daniel,</div><div><br></div><div>Maybe my commit message was not clear. The idea is that the SelectionDAG store vectorizer can only handle pairs. So, the number three means "more than a pair". </div><div><br></div><div>Thanks,</div><div>Nadav<br><br>Sent from my iPhone</div><div><div class="h5"><div><br>On Jul 26, 2013, at 17:48, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>> wrote:<br><br></div><blockquote type="cite"><div dir="ltr">Hey Nadav,<div>I'd humbly suggest that rather than use 3 directly, you should add a shared constant between these two passes, so when one changes, the other doesn't need to be updated. It would also ensure this bit of info about what needs to be updated isn't only contained in the comments.. </div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 26, 2013 at 4:07 PM, Nadav Rotem<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span style="font-family: arial, sans-serif; font-size: 13px;">Author: nadav</span><br style="font-family: arial, sans-serif; font-size: 13px;"><span style="font-family: arial, sans-serif; font-size: 13px;">Date: Fri Jul 26 18:07:55 2013</span><br style="font-family: arial, sans-serif; font-size: 13px;"><span style="font-family: arial, sans-serif; font-size: 13px;">New Revision: 187267</span><br style="font-family: arial, sans-serif; font-size: 13px;"><br style="font-family: arial, sans-serif; font-size: 13px;"><span style="font-family: arial, sans-serif; font-size: 13px;">URL: </span><a href="http://llvm.org/viewvc/llvm-project?rev=187267&view=rev" target="_blank" style="font-family: arial, sans-serif; font-size: 13px;">http://llvm.org/viewvc/llvm-project?rev=187267&view=rev</a><br style="font-family: arial, sans-serif; font-size: 13px;"><span style="font-family: arial, sans-serif; font-size: 13px;">Log:</span><br style="font-family: arial, sans-serif; font-size: 13px;"><span style="font-family: arial, sans-serif; font-size: 13px;">SLP</span><span style="font-family: arial, sans-serif; font-size: 13px;"> Vectorier: Don't </span><span style="font-family: arial, sans-serif; font-size: 13px;">vectorize</span><span style="font-family: arial, sans-serif; font-size: 13px;"> really short chains because they are already handled by the SelectionDAG store-</span><span style="font-family: arial, sans-serif; font-size: 13px;">vectorizer</span><span style="font-family: arial, sans-serif; font-size: 13px;">, which does a better job in deciding when to </span><span style="font-family: arial, sans-serif; font-size: 13px;">vectorize</span><span style="font-family: arial, sans-serif; font-size: 13px;">.</span></blockquote><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><br>Modified: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=187267&r1=187266&r2=187267&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=187267&r1=187266&r2=187267&view=diff</a><br>==============================================================================<br>--- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)<br>+++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Fri Jul 26 18:07:55 2013<br>@@ -898,8 +898,12 @@ int BoUpSLP::getTreeCost() {<br> DEBUG(dbgs() << "SLP: Calculating cost for tree of size " <<<br> VectorizableTree.size() << ".\n");<br><br>- if (!VectorizableTree.size()) {<br>- assert(!ExternalUses.size() && "We should not have any external users");<br>+ // Don't vectorize tiny trees. Small load/store chains or consecutive stores<br>+ // of constants will be vectoried in SelectionDAG in MergeConsecutiveStores.<br>+ if (VectorizableTree.size() < 3) {<br>+ if (!VectorizableTree.size()) {<br>+ assert(!ExternalUses.size() && "We should not have any external users");<br>+ }<br> return 0;<br> }</blockquote></div></div></div></blockquote></div></div></div></blockquote></div></div></div></blockquote></div><br></body></html>