<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">+ V = DFT.back();</div><div class="">+ DFT.pop_back();</div><div class=""><br class=""></div><div class="">You can also use "V = DFT.pop_back_val();"</div><div class=""><br class=""></div><div class="">Go ahead and commit the first patch with that change, so that we can stop exploding the stack.</div><div class=""><br class=""></div><div class="">For the second patch:</div><div class=""><br class=""></div><div class=""><div class="">+// Return false on failure. On succes, the Value the comparison matched against</div></div><div class=""><br class=""></div><div class="">s/succes/success</div><div class=""><br class=""></div><div class=""><div class="">+// Results.Vals with the set of value that match (or doesn't depending on isEQ).</div><div class="">+// Return false on failure. On succes, the Value the comparison matched against</div><div class="">+// is place in Result.CompValue.</div><div class="">+// If Result.CompValue is already used, the function is expected to fail if a</div><div class="">+// match is found but the value compared to is not the one expected.</div><div class="">+bool GatherConstantComparesMatch(Instruction *I, const DataLayout *DL,</div><div class="">+ bool isEQ,</div><div class="">+ GatherConstantComparesResult &Result) {</div></div><div class=""><br class=""></div><div class="">Seems like this should be a method on the GatherConstantComparesResult struct, as it’s just adjusting the struct’s state.</div><div class=""><br class=""></div><div class=""><div class="">+bool GatherConstantComparesMatch(Instruction *I, const DataLayout *DL,</div></div><div class=""><br class=""></div><div class="">Similarly here.</div><div class=""><br class=""></div><div class="">How about we call the struct something like ConstantComparesGatherer, and it has methods called “match” and “gather”, or else something more descriptive. Without the “ConstantComparesGather*” redundancy, we can make the mehod names clearer.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""></div><div><blockquote type="cite" class=""><div class="">On Nov 17, 2014, at 8:17 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Here is the patch updated without the #include <stack></div><div class=""><br class=""></div><div class="">Bonus is the second patch to implement the refactor you suggested.</div><div class=""><br class=""></div><div class="">Mehdi</div><div class=""><br class=""></div><div class=""><br class=""></div></div><span id="cid:D2D23C36-8A64-4E14-B9ED-0203CBA7B96F@apple.com"><0001-SimplifyCFG-turn-recursive-GatherConstantCompares-in.patch></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""></div><span id="cid:7EF51C26-5C7A-495D-AC08-B271971B0170@apple.com"><0002-SimplifyCFG-Refactor-GatherConstantCompares-result-i.patch></span><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Nov 17, 2014, at 1:59 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" class="">milseman@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Ah, of course. Drop the "#include <stack>” and the patch LGTM. You could also consider refactoring into a class to track all the state that’s flying around in a subsequent commit, but I’m glad the stack-explosion problem is addressed.<div class=""><br class=""><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Nov 17, 2014, at 1:51 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><blockquote type="cite" 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;" class=""><br class="Apple-interchange-newline">On Nov 17, 2014, at 1:48 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" class="">milseman@apple.com</a>> wrote:<br class=""><br class="">+ V = DFT.back();<br class="">+ DFT.pop_back();<br class=""><br class="">V = DFT.pop_back_val();<br class=""><br class="">+// one expected. If CurrValue is supplied, the return value has to be either<br class="">+// nullptr or CurrValue<br class="">...<br class="">+ // Try to match the current instruction<br class="">+ if (Value *Matched = GatherConstantComparesMatch(I,<br class="">+ CurrValue,<br class="">+ Vals,<br class="">+ DL,<br class="">+ UsedICmps,<br class="">+ isEQ)) {<br class="">+ // Match succeed, continue the loop<br class="">+ CurrValue = Matched;<br class=""><br class="">If that comment is true, then isn’t this assignment meaningless? That is, if the return is null then we’d never do the assignment. If the return is non-null, then it must be the same as the old CurrValue. Perhaps change this to an assert? If this is the case, then I wonder if GatherConstantComparesMatch shouldn’t just return a bool.<br class=""></blockquote><br 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;" class=""><span 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; float: none; display: inline !important;" class="">The first time GatherConstantComparesMatch is called, CurrValue is nullptr and we need to initialize CurrValue.</span><br 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;" class=""><span 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; float: none; display: inline !important;" class="">I can add an assert like :</span><br 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;" class=""><br 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;" class=""><span 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; float: none; display: inline !important;" class="">assert(not CurrValue || CurrValue == Matched);</span><br 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;" class=""><br 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;" class=""><span 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; float: none; display: inline !important;" class="">(but the assignment is still meaningful).</span><br 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;" class=""><br 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;" class=""><span 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; float: none; display: inline !important;" class="">Mehdi</span><br 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;" class=""><br 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;" class=""><br 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;" class=""><br 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;" class=""><blockquote type="cite" 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;" class=""><br class=""><br class=""><blockquote type="cite" class="">On Nov 17, 2014, at 1:40 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>> wrote:<br class=""><br class=""><br class=""><blockquote type="cite" class="">On Nov 17, 2014, at 12:24 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" class="">milseman@apple.com</a>> wrote:<br class=""><br class="">I like the change, but have a few minor comments:<br class="">I would replace the stack with a SmallVector, as this code tends to return early once we start exceeding 8 entries (else the Span is too large).<br class=""></blockquote><br class="">It is not exactly the case, the limit for the Span is for a single comparison. We can have an infinite sequence of or/and chained.<br class="">However it is true that the common case it probably a short chain. So I replaced the stack with a SmallVector, but the vector of matched value (that will be turned as the cases of the switch) as well.<br class=""><br class=""><br class=""><blockquote type="cite" class=""><br class="">+// Try to match Instruction I as a comparison against a constant and populates<br class="">+// Vals with the set of value that match (or does not depending on isEQ).<br class="">+// Return nullptr on failure, or return the Value the comparison matched against<br class="">+// on success<br class="">+static Value* GatherConstantComparesMatch(Instruction *I,<br class="">+ Value *CurrValue,<br class=""><br class="">It took me a while of reading the code to realize what CurrValue as a parameter (as opposed to a return value) is for. You could briefly mention it in the comment before the function. It also seems like CurrValue, if set, will also always be the return value on success. Does it make sense to instead have that book-keeping be done in the caller of GatherConstantComparesMatch? Also, why does GatherConstantComparesMatch take in Extra?<span class="Apple-converted-space"> </span><br class=""></blockquote><br class="">I add a comment, I can’t keep it out of the function because it changes the control flow inside.<br class="">I removed Extra, it was unused, well spotted!<br class=""><br class="">Attached is the update patch.<br class=""><br class=""><0001-SimplifyCFG-turn-recursive-GatherConstantCompares-in.patch><br class=""><br class=""><br class="">Mehdi<br class=""><br class=""><br class=""><br class=""><blockquote type="cite" class=""><br class=""><br class=""><blockquote type="cite" class="">On Nov 17, 2014, at 11:25 AM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>> wrote:<br class=""><br class="">Hi all,<br class=""><br class="">A long sequence of || or && of integer comparison against the same value is turned into a switch in SimplifyCFG.<br class=""><br class="">The implementation was recursive and could lead to a stack explosion. I turned it into an iterative implementation in this patch.<br class=""><br class="">As a side question, the existing code assumes that the “icmp" are always taking the constant as the second operand (it never tries to match the first operand). Is it something that is canonicalized in the IR and can be assumed here?<br class=""><br class="">Thanks,<br class=""><br class="">Mehdi<br class=""><br class=""><br class=""><0001-SimplifyCFG-turn-recursive-GatherConstantCompares-in.patch>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></blockquote></blockquote></blockquote></div></blockquote></div><br class=""></div></div></div></div></blockquote></div><br class=""></div></div></div></blockquote></div><br class=""></body></html>