<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:2.0cm 42.5pt 2.0cm 3.0cm;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="RU" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">Hi Roman,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">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></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">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></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">Max<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif"> rtereshin@apple.com [mailto:rtereshin@apple.com]
<br>
<b>Sent:</b> Friday, August 17, 2018 1:22 PM<br>
<b>To:</b> Maxim Kazantsev <max.kazantsev@azul.com><br>
<b>Cc:</b> sanjoy@playingwithpointers.com; listmail@philipreames.com; javed.absar@arm.com; llvm-commits <llvm-commits@lists.llvm.org>; junbuml@codeaurora.org; wmi@google.com; marcello.maggioni@gmail.com; Justin Bogner <jbogner@apple.com><br>
<b>Subject:</b> Re: [PATCH] D40645: [SCEV][NFC] Check NoWrap flags before lexicographical comparison of SCEVs<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:10.0pt">Hi all,<br>
<br>
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>
With the patch reverted the issue goes away.<br>
<br>
Please run<br>
<br>
$ opt -load-store-vectorizer reproducer.ll -S -o -<br>
<br>
to see the difference in vectorization.<br>
<br>
If the following patch is also applied:<br>
<br>
diff --git a/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp b/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp<br>
index e2ee87b2086..74be4d4c87b 100644<br>
--- a/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp<br>
+++ b/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp<br>
@@ -445,6 +445,12 @@ bool Vectorizer::lookThroughComplexAddresses(Value *PtrA, Value *PtrB,<br>
const SCEV *OffsetSCEVB = SE.getSCEV(OpB);<br>
const SCEV *C = SE.getConstant(IdxDiff.trunc(BitWidth));<br>
const SCEV *X = SE.getAddExpr(OffsetSCEVA, C);<br>
+ if (OpA->getName() == "preheader.address0.idx" &&<br>
+ OpB->getName() == "common.address.idx") {<br>
+ OffsetSCEVA->dump();<br>
+ X->dump();<br>
+ OffsetSCEVB->dump();<br>
+ }<br>
return X == OffsetSCEVB;<br>
}<br>
<br>
you will see the following output as well:<br>
<br>
(((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>
(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>
(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>
<br>
As you can see, second and third SCEVAddExpr's are the same but sorted differently.<br>
<br>
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>
for instance, getAddExpr(1, ((A * B) + (C * D))) returns (1 + (C * D) + (A * B)).<br>
<br>
<o:p></o:p></span></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:10.0pt"><br>
<br>
<br>
Thanks,<br>
Roman<br>
<br>
<br>
<br>
target triple = "x86_64--"<br>
<br>
@global_value0 = external constant i32<br>
@global_value1 = external constant i32<br>
@other_value = external global float<br>
@a = external global float<br>
@b = external global float<br>
@c = external global float<br>
@d = external global float<br>
@plus1 = external global i32<br>
@cnd = external global i8<br>
<br>
; Function Attrs: nounwind<br>
define void @main() local_unnamed_addr #0 {<br>
entry:<br>
%tmp = load i32, i32* @global_value0, !range !0<br>
%tmp2 = load i32, i32* @global_value1<br>
%and.i.i = and i32 %tmp2, 2<br>
%add.nuw.nsw.i.i = add nuw nsw i32 %and.i.i, 0<br>
%mul.i.i = shl nuw nsw i32 %add.nuw.nsw.i.i, 1<br>
%and6.i.i = and i32 %tmp2, 3<br>
%and9.i.i = and i32 %tmp2, 4<br>
%add.nuw.nsw10.i.i = add nuw nsw i32 %and6.i.i, %and9.i.i<br>
%conv3.i42.i = add nuw nsw i32 %mul.i.i, 1<br>
%reass.add346.7 = add nuw nsw i32 %add.nuw.nsw10.i.i, 56<br>
%reass.mul347.7 = mul nuw nsw i32 %tmp, %reass.add346.7<br>
%add7.i.7 = add nuw nsw i32 %reass.mul347.7, 0<br>
%preheader.address0.idx = add nuw nsw i32 %add7.i.7, %mul.i.i<br>
%preheader.address0.idx.zext = zext i32 %preheader.address0.idx to i64<br>
%preheader.load0.address = getelementptr inbounds float, float* @other_value, i64 %preheader.address0.idx.zext<br>
%preheader.load0. = load float, float* %preheader.load0.address, align 4, !tbaa !1<br>
%common.address.idx = add nuw nsw i32 %add7.i.7, %conv3.i42.i<br>
%preheader.header.common.address.idx.zext = zext i32 %common.address.idx to i64<br>
%preheader.load1.address = getelementptr inbounds float, float* @other_value, i64 %preheader.header.common.address.idx.zext<br>
%preheader.load1. = load float, float* %preheader.load1.address, align 4, !tbaa !1<br>
br label %for.body23<br>
<br>
for.body23: ; preds = %for.body23, %entry<br>
%loop.header.load0.address = getelementptr inbounds float, float* @other_value, i64 %preheader.header.common.address.idx.zext<br>
%loop.header.load0. = load float, float* %loop.header.load0.address, align 4, !tbaa !1<br>
%reass.mul343.7 = mul nuw nsw i32 %reass.add346.7, 72<br>
%add7.i286.7.7 = add nuw nsw i32 %reass.mul343.7, 56<br>
%add9.i288.7.7 = add nuw nsw i32 %add7.i286.7.7, %mul.i.i<br>
%loop.header.address1.idx = add nuw nsw i32 %add9.i288.7.7, 1<br>
%loop.header.address1.idx.zext = zext i32 %loop.header.address1.idx to i64<br>
%loop.header.load1.address = getelementptr inbounds float, float* @other_value, i64 %loop.header.address1.idx.zext<br>
%loop.header.load1. = load float, float* %loop.header.load1.address, align 4, !tbaa !1<br>
store float %preheader.load0., float* @a, align 4, !tbaa !1<br>
store float %preheader.load1., float* @b, align 4, !tbaa !1<br>
store float %loop.header.load0., float* @c, align 4, !tbaa !1<br>
store float %loop.header.load1., float* @d, align 4, !tbaa !1<br>
%loaded.cnd = load i8, i8* @cnd<br>
%condition = trunc i8 %loaded.cnd to i1<br>
br i1 %condition, label %for.body23, label %exit<br>
<br>
exit:<br>
ret void<br>
}<br>
<br>
attributes #0 = { nounwind }<br>
<br>
!0 = !{i32 0, i32 65536}<br>
!1 = !{!2, !2, i64 0}<br>
!2 = !{!"float", !3, i64 0}<br>
!3 = !{!"omnipotent char", !4, i64 0}<br>
!4 = !{!"Simple C++ TBAA"}<br>
<br>
<br>
> On Aug 10, 2018, at 5:13 PM, Roman Tereshin via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br>
> <br>
> rtereshin added inline comments.<br>
> Herald added a subscriber: javed.absar.<br>
> <br>
> <br>
> ================<br>
> Comment at: llvm/trunk/lib/Analysis/ScalarEvolution.cpp:699<br>
> + if (LA->getNoWrapFlags() != RA->getNoWrapFlags())<br>
> + return (int)LA->getNoWrapFlags() - (int)RA->getNoWrapFlags();<br>
> +<br>
> ----------------<br>
> Hi everyone,<br>
> <br>
> 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>
> <br>
> 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>
> <br>
> 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>
> <br>
> 2 identical SCEV's won't compare equal by pointer comparison as they are supposed to.<br>
> <br>
> I didn't verify this with an experiment yet, please correct me if I'm wrong.<br>
> <br>
> Thanks,<br>
> Roman<br>
> <br>
> <br>
> Repository:<br>
> rL LLVM<br>
> <br>
> <a href="https://reviews.llvm.org/D40645">https://reviews.llvm.org/D40645</a><br>
> <br>
> <br>
> <o:p></o:p></span></p>
</div>
</div>
</div>
</body>
</html>