<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:dt="uuid:C2F41010-65B3-11d1-A29F-00AA00C14882" 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 12 (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;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
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;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.25in 1.0in 1.25in;}
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=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Hi Nick, <o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I fixed #3 (ASM parser for constant pointer vectors) and added the testcase that you provided. <o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I do plan to add support for “%X = load <4 x i8*> %ptr” one day. But, first, I plan to add scatter/gather intrinsic. I expect vector load/store to require many changes all over the place. <o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I think that I fixed all of your comments. Please let me know if you see other problems with the patch. Thanks again for reviewing my patch.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Nadav<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><p class=MsoNormal><b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> Nick Lewycky [mailto:nlewycky@google.com] <br><b>Sent:</b> Wednesday, November 30, 2011 00:57<br><b>To:</b> Rotem, Nadav<br><b>Cc:</b> llvm-commits@cs.uiuc.edu<br><b>Subject:</b> Re: Updated pointer-vector patch<o:p></o:p></span></p></div><p class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>On 29 November 2011 10:27, Rotem, Nadav <<a href="mailto:nadav.rotem@intel.com">nadav.rotem@intel.com</a>> wrote:<o:p></o:p></p><p class=MsoNormal>Hi Nick,<br><br>Thank you for reviewing my code. I made corrections, and prepared a new version. I made the following changes:<br><br>1. I added support for icmp of pointer-vectors.<br>2. Made corrections in LangRef (inbound, summary, inttoptr example, etc).<br>3. Added additional verifications (vector width, etc)<br>4. Changed the casts to dyn_casts.<br>5. Simplified the verification code that you said was confusing.<br>6. Documented getBitWidth.<br>7. I went over the users of getIndexedOffset, and changed some of them to abort the optimization if a pointer-vector is used.<br>8. Fixed IndVarSimplify.<o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Excellent, thanks! Unfortunately I'm rather busy today and am unlikely to be able to finish reviewing it before tomorrow. Sorry.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>One high-level question. Are you planning to permit:<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><div><p class=MsoNormal> %X = load <4 x i8*> %ptr<o:p></o:p></p></div></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>eventually? I'd been assuming you were, but I realized that I should confirm that. When you do, you'll need to audit every place in LLVM that looks at the pointer argument of a load or store and assumes that it has a pointer (since now it may have a vector).<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><p class=MsoNormal>What I did not fix:<br><br>1. I did not add optimizations to ConstantFold. I plan to add these in the future.<o:p></o:p></p></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Okay. The patch can land first, but this is high priority thereafter. What ConstantFold (and ConstantFolding in lib/Analysis) must not do is crash, assert or produce invalid IR.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><p class=MsoNormal>2. I did not fix all of the users of getIndexedOffset. Currently there is an assertion in getIndexOffsets. If the fixes I made are not enough then I will fix additional users.<o:p></o:p></p></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>This might or might not be okay. I'll try to create a testcase which crashes due to this and send it to you if successful. If I can't then you're probably fine. :)<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><p class=MsoNormal>3. Did not add support to the ASM parser for constant pointer vectors. I may add this feature in the future.<o:p></o:p></p></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Sorry, but this one needs to happen. The issue is that a module can contain these -- they can be created through the API, will pass the module verifier, can be serialized and deserialized in .bc files, and will print out a .ll that we then can't reparse.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Here's a testcase:<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal> ; RUN: opt -instcombine -S -o - %s | llvm-as<o:p></o:p></p></div><div><div><p class=MsoNormal> ; RUN: opt -instcombine -globalopt -S -o - %s | llvm-as<o:p></o:p></p></div></div><div><p class=MsoNormal> @G1 = global i32 zeroinitializer<o:p></o:p></p></div><div><p class=MsoNormal> @G2 = global i32 zeroinitializer<o:p></o:p></p></div><div><p class=MsoNormal> @g = global <2 x i32*> zeroinitializer<o:p></o:p></p></div><div><p class=MsoNormal> %0 = type { i32, void ()* }<o:p></o:p></p></div><div><p class=MsoNormal> @llvm.global_ctors = appending global [1 x %0] [%0 { i32 65535, void ()* @test }]<o:p></o:p></p></div><div><p class=MsoNormal> define internal void @test() {<o:p></o:p></p></div><div><div><p class=MsoNormal> %A = insertelement <2 x i32*> undef, i32* @G1, i32 0<o:p></o:p></p></div><div><p class=MsoNormal> %B = insertelement <2 x i32*> %A, i32* @G2, i32 1<o:p></o:p></p></div></div><div><p class=MsoNormal> store <2 x i32*> %B, <2 x i32*>* @g<o:p></o:p></p></div><div><div><p class=MsoNormal> ret void<o:p></o:p></p></div><div><p class=MsoNormal> }<o:p></o:p></p></div></div><div><p class=MsoNormal><o:p> </o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><p class=MsoNormal>4. I did not add unit tests. Was this related to the constant ?<o:p></o:p></p></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Hm, we don't have any type tests already. I'll keep a list of anything I run across that I think really needs a test.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Nick<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><p class=MsoNormal><br>I attached the revised patch.<br><br>Thanks,<br>Nadav<o:p></o:p></p><div><p class=MsoNormal style='margin-bottom:12.0pt'><br><br>-----Original Message-----<br>From: Nick Lewycky [mailto:<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>]<br>Sent: Tuesday, November 29, 2011 04:39<br>To: Rotem, Nadav<br>Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>Subject: Re: Updated pointer-vector patch<o:p></o:p></p></div><div><div><p class=MsoNormal>On 28 November 2011 12:33, Rotem, Nadav <<a href="mailto:nadav.rotem@intel.com">nadav.rotem@intel.com</a>> wrote:<br>><br>> I attached the updated pointer-vector patch, following comments from Nick Lewycky.<br><br>To elaborate, the comment was that the only pointer types we should<br>allow are those which would let us load the vector and produce a legal<br>vector type.<br><br>+<p>In cases where the pointer argument is a vector of pointers, only a<br>+ single index may be used. For example: </p><br>+<pre class="doc_code"><br>+ %A = getelementptr <4 x i8*> %ptrs, <4 x i64> %offsets,<br>+</pre><br>Also mention that the number of elements must be the same.<br>- %X = ptrtoint i32* %X to i8 <i>; yields truncation on<br>32-bit architecture</i><br>- %Y = ptrtoint i32* %x to i64 <i>; yields zero extension on<br>32-bit architecture</i><br>+ %X = ptrtoint i32* %X to i8 <i>; yields<br>truncation on 32-bit architecture</i><br><br>While you're here, please make this use "%x" (lowercase) as the<br>operand consistently. The existing code "%X = op %X" is bad form<br>(using its own result).<br><br>Also update the overview and summary for the instruction, replace<br>"pointer" with "pointer or vector of pointers" and "integer" with<br>"integer or vector of integers". See the documentation of "add" for<br>example. The semantics can simply mention that for a vector it does<br>the operation elementwise.<br><br>Same with inttoptr and bitcast.<br><br> template <typename IndexTy><br> static Type *getIndexedTypeInternal(Type *Ptr, ArrayRef<IndexTy> IdxList) {<br>+ if (Ptr->isVectorTy()) {<br>+ IndexTy Index = IdxList[0];<br>"Index" is a dead variable right here. Did you mean to use it?<br>+unsigned GetElementPtrInst::getAddressSpace(Value *Ptr) {<br>+ Type* Ty = Ptr->getType();<br>"Type* Ty" --> "Type *Ty".<br>+ if (Ty->isVectorTy())<br>+ Ty = cast<VectorType>(Ty)->getElementType();<br>+<br>+ if (Ty->isPointerTy())<br>+ return cast<PointerType>(Ty)->getAddressSpace();<br><br>Please use "if (VectorType *VTy = dyn_cast<VectorType>(Ty)) Ty =<br>VTy->getElementTy();". The difference is that when compiled in debug<br>mode, the code you wrote will do two tests.<br><br>+ Assert1(SrcTy->isVectorTy() == DestTy->isVectorTy(),<br>+ "PtrToInt type mismatch", &I);<br><br>Please also verify the number of elements. Again in IntToPtr.<br><br> Type *ElTy =<br> GetElementPtrInst::getIndexedType(GEP.getOperand(0)->getType(), Idxs);<br> Assert1(ElTy, "Invalid indices for GEP pointer type!", &GEP);<br>- Assert2(GEP.getType()->isPointerTy() &&<br>+ if (GEP.getOperand(0)->getType()->isPointerTy()) {<br>+ Assert2(GEP.getType()->isPointerTy() &&<br> cast<PointerType>(GEP.getType())->getElementType() == ElTy,<br> "GEP is not of right type for indices!", &GEP, ElTy);<br>+ } else {<br>+ Assert1(Idxs.size() == 1, "Invalid number of indices!", &GEP);<br><br>I'm confused. Can this assert ever fire when "Invalid indices for GEP<br>pointer type!" doesn't?<br><br> bool VectorType::isValidElementType(Type *ElemTy) {<br>+ if (ElemTy->isPointerTy())<br>+ ElemTy = cast<PointerType>(ElemTy)->getElementType();<br> return ElemTy->isIntegerTy() || ElemTy->isFloatingPointTy();<br> }<br><br>Again, please use dyn_cast.<br><br><br>The changes in this patch fail to address vectors of pointers in an<br>icmp operation. It's fine if you want to leave that illegal and add<br>support for it later. The verifier will reject vectors of pointers as<br>icmp arguments, while continuing to accept integer vectors and scalar<br>pointers.<br><br>The change to the LangRef doesn't address the meaning of inbounds on a<br>vector-gep. It seems reasonable to say that it applies to each of the<br>computations element-wise.<br><br>You need to update the comment on VectorType::getBitWidth to indicate<br>that it will return zero on vectors of pointers.<br><br>You need to update TargetData::getIndexedOffset to handle a vector<br>index, or update all of its callers. I don't know how to do this<br>because it returns a single integer, but you really want something<br>element-wise.<br><br>Currently SROA will bail on "alloca <4 x i8*>". It (and mem2reg) need<br>to be taught to handle them. This is important for performance, but<br>can hold off until later.<br><br>This code in ConstantFold.cpp:<br><br>// If the right hand side is a bitcast, try using its inverse to simplify<br>// it by moving it to the left hand side. We can't do this if it would turn<br>// a vector compare into a scalar compare or visa versa.<br>if (ConstantExpr *CE2 = dyn_cast<ConstantExpr>(C2)) {<br> Constant *CE2Op0 = CE2->getOperand(0);<br> if (CE2->getOpcode() == Instruction::BitCast &&<br> CE2->getType()->isVectorTy() == CE2Op0->getType()->isVectorTy()) {<br> Constant *Inverse = ConstantExpr::getBitCast(C1, CE2Op0->getType());<br> return ConstantExpr::getICmp(pred, Inverse, CE2Op0);<br> }<br>}<br><br>assumes that it can produce a bitcast/icmp of any vector you can<br>bitcast. Now that we permit bitcasting on vectors of pointers, that is<br>no longer true. Please write a testcase, or let me know if you can't.<br><br>The ASM parser doesn't parse constant vectors of pointers. Testcase:<br><br> // llvm-as < %s -disable-output<br> @G1 = global i32 zeroinitializer<br> @G2 = global i32 zeroinitializer<br> @g = constant <2 x i32*> <i32* @G1, i32* @G2><br><br>FoldBitCast in ConstantFolding.cpp should be taught that it can create<br>vectors of pointers too:<br><br> // If this is a scalar -> vector cast, convert the input into a <1 x scalar><br> // vector so the code below can handle it uniformly.<br> if (isa<ConstantFP>(C) || isa<ConstantInt>(C)) {<br><br>|| C->getType() is pointer to a vector argument.<br><br> Constant *Ops = C; // don't take the address of C!<br> return FoldBitCast(ConstantVector::get(Ops), DestTy, TD);<br> }<br><br>You should add tests for the basic correctness to tests under<br>unittests/. Show that creating a vector of null pointers becomes a<br>constant aggregate zero, show that you can create a GEP with a<br>non-pointer argument (the vector).<br><br>This code inside IndVarSimplify.cpp needs to change<br><br> // Retrieve the pointer operand of the GEP. Don't use GetUnderlyingObject<br> // because it understands lcssa phis while SCEV does not.<br> Value *FromPtr = FromVal;<br> Value *ToPtr = ToVal;<br> if (GEPOperator *GEP = dyn_cast<GEPOperator>(FromVal)) {<br> FromPtr = GEP->getPointerOperand();<br> }<br> if (GEPOperator *GEP = dyn_cast<GEPOperator>(ToVal)) {<br> ToPtr = GEP->getPointerOperand();<br> }<br><br>because it sends the FromPtr and ToPtr to SCEV:<br><br> const SCEV *FromBase = SE->getPointerBase(SE->getSCEV(FromPtr));<br> const SCEV *ToBase = SE->getPointerBase(SE->getSCEV(ToPtr));<br><br>which does not support vectors of pointers, only normal pointers.<br><br>Nick<br><br>><br>> Thanks,<br>> Nadav<br>><br>> ---------------------------------------------------------------------<br>> Intel Israel (74) Limited<br>><br>> This e-mail and any attachments may contain confidential material for<br>> the sole use of the intended recipient(s). Any review or distribution<br>> by others is strictly prohibited. If you are not the intended<br>> recipient, please contact the sender and delete all copies.<br>---------------------------------------------------------------------<br>Intel Israel (74) Limited<br><br>This e-mail and any attachments may contain confidential material for<br>the sole use of the intended recipient(s). Any review or distribution<br>by others is strictly prohibited. If you are not the intended<br>recipient, please contact the sender and delete all copies.<o:p></o:p></p></div></div></blockquote></div><p class=MsoNormal><o:p> </o:p></p></div><font face="monospace">---------------------------------------------------------------------<br>
Intel Israel (74) Limited<br>
<br>
This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</font></body></html>