<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 Maxim,<div class=""><br class=""></div><div class="">+ Eli Friedman<br class=""><div class=""><br class=""></div><div class="">Thank you for a quick response!</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="WordSection1" style="page: WordSection1;"><div class="" style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;"><span lang="EN-US" class="" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">and the fact that we may end up assigining NoWrap flags after the SCEV was created is extremely annoying by many means</span></div></div></blockquote><div class=""><br class=""></div>I agree. I would also like to elaborate on this but a bit later.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="WordSection1" style="page: WordSection1;"><div class="" style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;"><span lang="EN-US" class="" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">We've put a lot of efforts to avoid creation of huge SCEVs</span></div></div></blockquote><div class=""><br class=""></div>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).</div><div class=""><br class=""></div><div 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).</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">For CompareSCEVComplexity / GroupByComplexity I do not have good benchmarks, unfortunately. I would assume you have some as something motivated your patch!</div><div class=""><div><br class=""></div><div><blockquote type="cite" class=""><div class="WordSection1" style="page: WordSection1;"><div class="" style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;"><span lang="EN-US" class="" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">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</span></div></div></blockquote><br class=""></div><div>Thank you!</div><div><br class=""></div><div>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.</div><div><br class=""></div><div>I would need some benchmarks to evaluate the impact however. Would greatly appreciate if you could find something in your IR archives.</div><div><br class=""></div><div>Thank you,</div><div>Roman</div><div><br class=""></div><div><br class=""><blockquote type="cite" class=""><div 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:</div><br class="Apple-interchange-newline"><div class=""><div class="WordSection1" style="page: WordSection1; 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;"><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" 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.<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div class=""><div style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(225, 225, 225); padding: 3pt 0cm 0cm;" class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><b class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif;" class="">From:</span></b><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span class="Apple-converted-space"> </span>Maxim Kazantsev<span class="Apple-converted-space"> </span><br class=""><b class="">Sent:</b><span class="Apple-converted-space"> </span>Friday, August 17, 2018 1:32 PM<br class=""><b class="">To:</b><span class="Apple-converted-space"> </span>'<a href="mailto:rtereshin@apple.com" class="">rtereshin@apple.com</a>' <<a href="mailto:rtereshin@apple.com" class="">rtereshin@apple.com</a>><br class=""><b class="">Cc:</b><span class="Apple-converted-space"> </span><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=""><b class="">Subject:</b><span class="Apple-converted-space"> </span>RE: [PATCH] D40645: [SCEV][NFC] Check NoWrap flags before lexicographical comparison of SCEVs<o:p class=""></o:p></span></div></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><o:p class=""> </o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Hi Roman,<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" 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.<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" 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.<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Thanks,<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Max<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div class=""><div style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(225, 225, 225); padding: 3pt 0cm 0cm;" class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><b class=""><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif;" class="">From:</span></b><span lang="EN-US" style="font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span class="Apple-converted-space"> </span><a href="mailto:rtereshin@apple.com" style="color: purple; text-decoration: underline;" class="">rtereshin@apple.com</a><span class="Apple-converted-space"> </span>[<a href="mailto:rtereshin@apple.com" style="color: purple; text-decoration: underline;" class="">mailto:rtereshin@apple.com</a>]<span class="Apple-converted-space"> </span><br class=""><b class="">Sent:</b><span class="Apple-converted-space"> </span>Friday, August 17, 2018 1:22 PM<br class=""><b class="">To:</b><span class="Apple-converted-space"> </span>Maxim Kazantsev <<a href="mailto:max.kazantsev@azul.com" style="color: purple; text-decoration: underline;" class="">max.kazantsev@azul.com</a>><br class=""><b class="">Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:sanjoy@playingwithpointers.com" style="color: purple; text-decoration: underline;" class="">sanjoy@playingwithpointers.com</a>;<span class="Apple-converted-space"> </span><a href="mailto:listmail@philipreames.com" style="color: purple; text-decoration: underline;" class="">listmail@philipreames.com</a>;<span class="Apple-converted-space"> </span><a href="mailto:javed.absar@arm.com" style="color: purple; text-decoration: underline;" class="">javed.absar@arm.com</a>; llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" style="color: purple; text-decoration: underline;" class="">llvm-commits@lists.llvm.org</a>>;<a href="mailto:junbuml@codeaurora.org" style="color: purple; text-decoration: underline;" class="">junbuml@codeaurora.org</a>;<span class="Apple-converted-space"> </span><a href="mailto:wmi@google.com" style="color: purple; text-decoration: underline;" class="">wmi@google.com</a>;<span class="Apple-converted-space"> </span><a href="mailto:marcello.maggioni@gmail.com" style="color: purple; text-decoration: underline;" class="">marcello.maggioni@gmail.com</a>; Justin Bogner <<a href="mailto:jbogner@apple.com" style="color: purple; text-decoration: underline;" class="">jbogner@apple.com</a>><br class=""><b class="">Subject:</b><span class="Apple-converted-space"> </span>Re: [PATCH] D40645: [SCEV][NFC] Check NoWrap flags before lexicographical comparison of SCEVs<o:p class=""></o:p></span></div></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: "Times New Roman", serif;" class=""><o:p class=""> </o:p></div><div class=""><div class=""><p class="MsoNormal" style="margin: 0cm 0cm 12pt; font-size: 12pt; font-family: "Times New Roman", serif;"><span style="font-size: 10pt;" 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)).<o:p class=""></o:p></span></p></div></div><div class=""><div class=""><p class="MsoNormal" style="margin: 0cm 0cm 12pt; font-size: 12pt; font-family: "Times New Roman", serif;"><span style="font-size: 10pt;" 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="">> On Aug 10, 2018, at 5:13 PM, Roman Tereshin via Phabricator <<a href="mailto:reviews@reviews.llvm.org" style="color: purple; text-decoration: underline;" class="">reviews@reviews.llvm.org</a>> wrote:<br class="">><span class="Apple-converted-space"> </span><br class="">> rtereshin added inline comments.<br class="">> Herald added a subscriber: javed.absar.<br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><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="">><span class="Apple-converted-space"> </span><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="">><span class="Apple-converted-space"> </span><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="">><span class="Apple-converted-space"> </span><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="">><span class="Apple-converted-space"> </span><br class="">> 2 identical SCEV's won't compare equal by pointer comparison as they are supposed to.<br class="">><span class="Apple-converted-space"> </span><br class="">> I didn't verify this with an experiment yet, please correct me if I'm wrong.<br class="">><span class="Apple-converted-space"> </span><br class="">> Thanks,<br class="">> Roman<br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">> Repository:<br class="">> rL LLVM<br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D40645" style="color: purple; text-decoration: underline;" class="">https://reviews.llvm.org/D40645</a><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span><br class="">><span class="Apple-converted-space"> </span></span></p></div></div></div></div></blockquote></div><br class=""></div></div></body></html>