<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Sanjoy,<div class=""><br class=""></div><div class="">Thanks for looking into this!</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span class="" style="float: none; display: inline !important;">- We can get rid of the scan in addToLoopUseLists by keeping a map</span><br class=""><span class="" style="float: none; display: inline !important;">(separately as a hashtable or a linked list of loops directly present</span><br class=""><span class="" style="float: none; display: inline !important;">in SCEV expressions) from SCEV expressions to the list of used loops.</span><br class=""><span class="" style="float: none; display: inline !important;">Then instead of doing a scan we can just look at the loops used by the</span><br class=""><span class="" style="float: none; display: inline !important;">immediate operations of a SCEV expression, S, to infer the loops used</span><br class=""><span class="" style="float: none; display: inline !important;">by S.</span></blockquote><div class=""><br class=""></div>Yes, this is what I had in mind here too, PTAL: <a href="https://reviews.llvm.org/D50985" class="">https://reviews.llvm.org/D50985</a></div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><span class="" style="float: none; display: inline !important;">- We can get rid of checkValidity by keeping a proper use list</span><br class=""><span class="" style="float: none; display: inline !important;">between SCEV expressions. Then when a SCEVUnknown is deleted, we can</span><br class=""><span class="" style="float: none; display: inline !important;">walk all of its transitive uses and tag them as invalid. What we do</span><br class=""><span class="" style="float: none; display: inline !important;">today (scan the entire expression for every SCEV) is kind of silly --</span><br class=""><span class="" style="float: none; display: inline !important;">we're making a very common operation slow to work around a fairly rare</span><br class=""><span class="" style="float: none; display: inline !important;">situation (SCEV preserved across value deletion). Once we have this</span><br class=""><span class="" style="float: none; display: inline !important;">we can reduce the memory used by LoopUsers. Today it tracks all</span><br class=""><span class="" style="float: none; display: inline !important;">transitive users of loops but with use lists it will only have to</span><br class=""><span class="" style="float: none; display: inline !important;">track AddRecs and we can construct the transitive set of users in</span><br class=""><span class="" style="float: none; display: inline !important;">ScalarEvolution::forgetLoop.</span></blockquote><div class=""><br class=""></div>True, I was thinking the same before I looked a little further into the motivation behind the patch that introduced checkValidity.</div><div class="">At the moment I'm not convinced a check like that is required at all unless there is a bug (an API mis-use) in a transformation</div><div class="">pass using Scalar Evolution, in this case, apparently, LSR.</div><div class=""><br class=""></div><div class="">I haven't looked into it enough to tell for sure, but the general line of reasoning is this: If a Value is deleted, all its Uses in IR</div><div class="">must be redirected (via RAUW) to other values or deleted / rendered unreachable themselves, which means SCEVCallbackVH::deleted() and</div><div class="">SCEVCallbackVH::allUsesReplacedWith(Value *V) should have been called for every SCEV node referencing (transitively)</div><div class="">the SCEVUnknown in question which is probably supposed to make that SCEVUnknown unreachable as well.</div><div class=""><br class=""></div><div class="">Roman</div><div class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Aug 18, 2018, at 11:46 AM, Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com" class="">sanjoy@playingwithpointers.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">On Fri, Aug 17, 2018 at 6:23 AM Roman Tereshin <</span><a href="mailto:rtereshin@apple.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">rtereshin@apple.com</a><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">> wrote:</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class="">Hi Maxim,<br class=""><br class="">+ Eli Friedman<br class=""><br class="">Thank you for a quick response!<br class=""><br class="">and the fact that we may end up assigining NoWrap flags after the SCEV was created is extremely annoying by many means<br class=""><br class=""><br class="">I agree. I would also like to elaborate on this but a bit later.<br class=""><br class="">We've put a lot of efforts to avoid creation of huge SCEVs<br class=""><br class=""><br class="">Feels like this is true, and also for wrong reasons. I found that asymptotically the worst offenders are getUsedLoops / addToLoopUseLists (O(n) where n is the number of nodes in a SCEV, called upon creation of each new SCEV node, a patch is coming soon to fix it) and checkValidity (O(n), called upon each getSCEV call if the result is cached, this one is trickier), having at least one of these around makes creating a large SCEV a O(n^2) deal (transformations are probably even more expensive, true, but they are limited by a small constant depth, while this n is not limited by anything).<br class=""></blockquote><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Some of this O(n^2) behavior can be fixed trading off memory for compile time:</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">- We can get rid of the scan in addToLoopUseLists by keeping a map</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">(separately as a hashtable or a linked list of loops directly present</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">in SCEV expressions) from SCEV expressions to the list of used loops.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Then instead of doing a scan we can just look at the loops used by the</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">immediate operations of a SCEV expression, S, to infer the loops used</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">by S.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">- We can get rid of checkValidity by keeping a proper use list</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">between SCEV expressions. Then when a SCEVUnknown is deleted, we can</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">walk all of its transitive uses and tag them as invalid. What we do</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">today (scan the entire expression for every SCEV) is kind of silly --</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">we're making a very common operation slow to work around a fairly rare</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">situation (SCEV preserved across value deletion). Once we have this</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">we can reduce the memory used by LoopUsers. Today it tracks all</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">transitive users of loops but with use lists it will only have to</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">track AddRecs and we can construct the transitive set of users in</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">ScalarEvolution::forgetLoop.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">I never got around to doing these because I didn't have enough time to</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">run the experiments we'd need to run to ensure that these won't</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">regress memory usage. Moreover, it wasn't clear to me //what// to</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">benchmark for compiler memory usage. But I'll be happy to review</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">patches if someone wants to take these on.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">-- Sanjoy</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class="">These are the reason why a patch like <a href="https://reviews.llvm.org/D32239" class="">https://reviews.llvm.org/D32239</a> ("[SCEV] Make SCEV or modeling more aggressive.") blows up compile time even when it does not cause a stack overflow. For these, however, I have good test cases to benchmark on (see <a href="https://bugs.llvm.org/show_bug.cgi?id=32731" class="">https://bugs.llvm.org/show_bug.cgi?id=32731</a> and more is coming).<br class=""><br class=""><br class="">For CompareSCEVComplexity / GroupByComplexity I do not have good benchmarks, unfortunately. I would assume you have some as something motivated your patch!<br class=""><br class="">it still makes sense to do this to save some compile time for adds/muls. Please feel free to either prohibit this for AddRecs or just revert this patch with adding a TODO that it still is a valid thing for add/muls<br class=""><br class=""><br class="">Thank you!<br class=""><br class="">I was thinking adding a simple ranking system (use a few free bits in each SCEV node to store / cache / memoize an integer complexity rank) and use it as a fast path as soon as the particular ordering is not important anymore (after the comparisons by a type and the loop dominance relation for AddRecs, etc.), resorting back to lexicographical comparison only if the ranks are equal.<br class=""><br class="">I would need some benchmarks to evaluate the impact however. Would greatly appreciate if you could find something in your IR archives.<br class=""><br class="">Thank you,<br class="">Roman<br class=""><br class=""><br class="">On Aug 16, 2018, at 11:35 PM, Maxim Kazantsev <<a href="mailto:max.kazantsev@azul.com" class="">max.kazantsev@azul.com</a>> wrote:<br class=""><br class="">UPD: I've looked into the code and we seem to do it only for AddRecs. I don't mind if you revert the patch with checking in your test.<br class=""><br class="">From: Maxim Kazantsev<br class="">Sent: Friday, August 17, 2018 1:32 PM<br class="">To: '<a href="mailto:rtereshin@apple.com" class="">rtereshin@apple.com</a>' <<a href="mailto:rtereshin@apple.com" class="">rtereshin@apple.com</a>><br class="">Cc: <a href="mailto:sanjoy@playingwithpointers.com" class="">sanjoy@playingwithpointers.com</a>; <a href="mailto:listmail@philipreames.com" class="">listmail@philipreames.com</a>; <a href="mailto:javed.absar@arm.com" class="">javed.absar@arm.com</a>; llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>>; <a href="mailto:junbuml@codeaurora.org" class="">junbuml@codeaurora.org</a>; <a href="mailto:wmi@google.com" class="">wmi@google.com</a>; <a href="mailto:marcello.maggioni@gmail.com" class="">marcello.maggioni@gmail.com</a>; Justin Bogner <<a href="mailto:jbogner@apple.com" class="">jbogner@apple.com</a>><br class="">Subject: RE: [PATCH] D40645: [SCEV][NFC] Check NoWrap flags before lexicographical comparison of SCEVs<br class=""><br class="">Hi Roman,<br class=""><br class="">Thanks for finding this issue. I think you are right, and the fact that we may end up assigining NoWrap flags after the SCEV was created is extremely annoying by many means. I think that the underlying problems against which it was made could've been fixed differently. We've put a lot of efforts to avoid creation of huge SCEVs.<br class=""><br class="">As far as I'm aware, we only set NoWraps flag lately for AddRecs and never for Adds/Muls. If it is so, it still makes sense to do this to save some compile time for adds/muls. Please feel free to either prohibit this for AddRecs or just revert this patch with adding a TODO that it still is a valid thing for add/muls and checking in your example.<br class=""><br class="">Thanks,<br class="">Max<br class=""><br class=""><br class="">From: <a href="mailto:rtereshin@apple.com" class="">rtereshin@apple.com</a> [<a href="mailto:rtereshin@apple.com" class="">mailto:rtereshin@apple.com</a>]<br class="">Sent: Friday, August 17, 2018 1:22 PM<br class="">To: Maxim Kazantsev <<a href="mailto:max.kazantsev@azul.com" class="">max.kazantsev@azul.com</a>><br class="">Cc: <a href="mailto:sanjoy@playingwithpointers.com" class="">sanjoy@playingwithpointers.com</a>; <a href="mailto:listmail@philipreames.com" class="">listmail@philipreames.com</a>; <a href="mailto:javed.absar@arm.com" class="">javed.absar@arm.com</a>; llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>>;<a href="mailto:junbuml@codeaurora.org" class="">junbuml@codeaurora.org</a>; <a href="mailto:wmi@google.com" class="">wmi@google.com</a>; <a href="mailto:marcello.maggioni@gmail.com" class="">marcello.maggioni@gmail.com</a>; Justin Bogner <<a href="mailto:jbogner@apple.com" class="">jbogner@apple.com</a>><br class="">Subject: Re: [PATCH] D40645: [SCEV][NFC] Check NoWrap flags before lexicographical comparison of SCEVs<br class=""><br class=""><br class="">Hi all,<br class=""><br class="">I just hit a real-world scenario where the problem I've described in the previous comment actually occurs and causes 2 identical SCEV expressions to have different order of operands and therefore compare not equal, which in its turn prevents LoadStoreVectorizer from vectorizing a pair of consecutive loads.<br class="">With the patch reverted the issue goes away.<br class=""><br class="">Please run<br class=""><br class="">$ opt -load-store-vectorizer reproducer.ll -S -o -<br class=""><br class="">to see the difference in vectorization.<br class=""><br class="">If the following patch is also applied:<br class=""><br class="">diff --git a/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp b/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp<br class="">index e2ee87b2086..74be4d4c87b 100644<br class="">--- a/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp<br class="">+++ b/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp<br class="">@@ -445,6 +445,12 @@ bool Vectorizer::lookThroughComplexAddresses(Value *PtrA, Value *PtrB,<br class=""> const SCEV *OffsetSCEVB = SE.getSCEV(OpB);<br class=""> const SCEV *C = SE.getConstant(IdxDiff.trunc(BitWidth));<br class=""> const SCEV *X = SE.getAddExpr(OffsetSCEVA, C);<br class="">+ if (OpA->getName() == "preheader.address0.idx" &&<br class="">+ OpB->getName() == "common.address.idx") {<br class="">+ OffsetSCEVA->dump();<br class="">+ X->dump();<br class="">+ OffsetSCEVB->dump();<br class="">+ }<br class=""> return X == OffsetSCEVB;<br class="">}<br class=""><br class="">you will see the following output as well:<br class=""><br class="">(((56 + (zext i2 (trunc i32 %tmp2 to i2) to i32) + (4 * (zext i1 (trunc i32 (%tmp2 /u 4) to i1) to i32))<nuw><nsw>) * %tmp) + (4 * (zext i1 (trunc i32 (%tmp2 /u 2) to i1) to i32))<nuw><nsw>)<br class="">(1 + ((56 + (zext i2 (trunc i32 %tmp2 to i2) to i32) + (4 * (zext i1 (trunc i32 (%tmp2 /u 4) to i1) to i32))<nuw><nsw>) * %tmp) + (4 * (zext i1 (trunc i32 (%tmp2 /u 2) to i1) to i32))<nuw><nsw>)<br class="">(1 + (4 * (zext i1 (trunc i32 (%tmp2 /u 2) to i1) to i32))<nuw><nsw> + ((56 + (zext i2 (trunc i32 %tmp2 to i2) to i32) + (4 * (zext i1 (trunc i32 (%tmp2 /u 4) to i1) to i32))<nuw><nsw>) * %tmp))<br class=""><br class="">As you can see, second and third SCEVAddExpr's are the same but sorted differently.<br class=""><br class="">On a larger example (the source of the reproducer.ll attached, which is a bugpoint) I have seen even weirder behavior: adding a constant to an existing SCEV changes the order of the existing terms,<br class="">for instance, getAddExpr(1, ((A * B) + (C * D))) returns (1 + (C * D) + (A * B)).<br class=""><br class=""><br class=""><br class=""><br class="">Thanks,<br class="">Roman<br class=""><br class=""><br class=""><br class="">target triple = "x86_64--"<br class=""><br class="">@global_value0 = external constant i32<br class="">@global_value1 = external constant i32<br class="">@other_value = external global float<br class="">@a = external global float<br class="">@b = external global float<br class="">@c = external global float<br class="">@d = external global float<br class="">@plus1 = external global i32<br class="">@cnd = external global i8<br class=""><br class="">; Function Attrs: nounwind<br class="">define void @main() local_unnamed_addr #0 {<br class="">entry:<br class=""> %tmp = load i32, i32* @global_value0, !range !0<br class=""> %tmp2 = load i32, i32* @global_value1<br class=""> %and.i.i = and i32 %tmp2, 2<br class=""> %add.nuw.nsw.i.i = add nuw nsw i32 %and.i.i, 0<br class=""> %mul.i.i = shl nuw nsw i32 %add.nuw.nsw.i.i, 1<br class=""> %and6.i.i = and i32 %tmp2, 3<br class=""> %and9.i.i = and i32 %tmp2, 4<br class=""> %add.nuw.nsw10.i.i = add nuw nsw i32 %and6.i.i, %and9.i.i<br class=""> %conv3.i42.i = add nuw nsw i32 %mul.i.i, 1<br class=""> %reass.add346.7 = add nuw nsw i32 %add.nuw.nsw10.i.i, 56<br class=""> %reass.mul347.7 = mul nuw nsw i32 %tmp, %reass.add346.7<br class=""> %add7.i.7 = add nuw nsw i32 %reass.mul347.7, 0<br class=""> %preheader.address0.idx = add nuw nsw i32 %add7.i.7, %mul.i.i<br class=""> %preheader.address0.idx.zext = zext i32 %preheader.address0.idx to i64<br class=""> %preheader.load0.address = getelementptr inbounds float, float* @other_value, i64 %preheader.address0.idx.zext<br class=""> %preheader.load0. = load float, float* %preheader.load0.address, align 4, !tbaa !1<br class=""> %common.address.idx = add nuw nsw i32 %add7.i.7, %conv3.i42.i<br class=""> %preheader.header.common.address.idx.zext = zext i32 %common.address.idx to i64<br class=""> %preheader.load1.address = getelementptr inbounds float, float* @other_value, i64 %preheader.header.common.address.idx.zext<br class=""> %preheader.load1. = load float, float* %preheader.load1.address, align 4, !tbaa !1<br class=""> br label %for.body23<br class=""><br class="">for.body23: ; preds = %for.body23, %entry<br class=""> %loop.header.load0.address = getelementptr inbounds float, float* @other_value, i64 %preheader.header.common.address.idx.zext<br class=""> %loop.header.load0. = load float, float* %loop.header.load0.address, align 4, !tbaa !1<br class=""> %reass.mul343.7 = mul nuw nsw i32 %reass.add346.7, 72<br class=""> %add7.i286.7.7 = add nuw nsw i32 %reass.mul343.7, 56<br class=""> %add9.i288.7.7 = add nuw nsw i32 %add7.i286.7.7, %mul.i.i<br class=""> %loop.header.address1.idx = add nuw nsw i32 %add9.i288.7.7, 1<br class=""> %loop.header.address1.idx.zext = zext i32 %loop.header.address1.idx to i64<br class=""> %loop.header.load1.address = getelementptr inbounds float, float* @other_value, i64 %loop.header.address1.idx.zext<br class=""> %loop.header.load1. = load float, float* %loop.header.load1.address, align 4, !tbaa !1<br class=""> store float %preheader.load0., float* @a, align 4, !tbaa !1<br class=""> store float %preheader.load1., float* @b, align 4, !tbaa !1<br class=""> store float %loop.header.load0., float* @c, align 4, !tbaa !1<br class=""> store float %loop.header.load1., float* @d, align 4, !tbaa !1<br class=""> %loaded.cnd = load i8, i8* @cnd<br class=""> %condition = trunc i8 %loaded.cnd to i1<br class=""> br i1 %condition, label %for.body23, label %exit<br class=""><br class="">exit:<br class=""> ret void<br class="">}<br class=""><br class="">attributes #0 = { nounwind }<br class=""><br class="">!0 = !{i32 0, i32 65536}<br class="">!1 = !{!2, !2, i64 0}<br class="">!2 = !{!"float", !3, i64 0}<br class="">!3 = !{!"omnipotent char", !4, i64 0}<br class="">!4 = !{!"Simple C++ TBAA"}<br class=""><br class=""><br class=""><blockquote type="cite" class="">On Aug 10, 2018, at 5:13 PM, Roman Tereshin via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""><br class="">rtereshin added inline comments.<br class="">Herald added a subscriber: javed.absar.<br class=""><br class=""><br class="">================<br class="">Comment at: llvm/trunk/lib/Analysis/ScalarEvolution.cpp:699<br class="">+ if (LA->getNoWrapFlags() != RA->getNoWrapFlags())<br class="">+ return (int)LA->getNoWrapFlags() - (int)RA->getNoWrapFlags();<br class="">+<br class="">----------------<br class="">Hi everyone,<br class=""><br class="">Unfortunately, wrapping flags are not a part of SCEV's identity (they do not participate in computing a hash value or in equality comparisons) and in fact they could be assigned after the fact w/o rebuilding a SCEV.<br class=""><br class="">At least, last time I checked it was that way. Grep for const_cast's to see quite a few of example, AFAIR all for AddRec's.<br class=""><br class="">So, if 2 expressions get built in 2 slightly different ways: one with flags set in the beginning, the other with the flags attached later on, we may end up with 2 expressions which are exactly the same but have their operands swapped in one of the commutative N-ary expressions, and at least one of them will have "sorted by complexity" invariant broken.<br class=""><br class="">2 identical SCEV's won't compare equal by pointer comparison as they are supposed to.<br class=""><br class="">I didn't verify this with an experiment yet, please correct me if I'm wrong.<br class=""><br class="">Thanks,<br class="">Roman<br class=""><br class=""><br class="">Repository:<br class="">rL LLVM<br class=""><br class=""><a href="https://reviews.llvm.org/D40645" class="">https://reviews.llvm.org/D40645</a></blockquote></blockquote></div></blockquote></div><br class=""></div></body></html>