<br><div class="gmail_quote">On 30 November 2011 13:03, Rotem, Nadav <span dir="ltr"><<a href="mailto:nadav.rotem@intel.com" target="_blank">nadav.rotem@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hi Nick, <u></u><u></u></span></p><p class="MsoNormal">
<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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. <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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. <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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.</span></p>
</div></div></blockquote><div><br></div><div>Thanks! I'm nearly happy with the things that your patch actually touches, that's the good news. I'm still really concerned about the remaining million lines of LLVM code which needs to be audited for as a result of the change. I'm trying to do that (as you noticed from my last email, but this round I found new problems) and I'm worried because the testcases that I write to trigger problems are all very simple.</div>
<div><br></div><div><div>+ /// GetGEPReturnType - Returns the pointer type returned by the GEP</div><div>+ /// instruction, which may be a vector of pointers.</div><div>+ static Type *getGEPReturnType(Value *Ptr, ArrayRef<Value *> IdxList) {</div>
<div>+ Type* PtrTy = PointerType::get(checkGEPType(</div></div><div><br></div><div>"Type *PtrTy".</div><div><br></div><div> template <typename IndexTy></div><div><div> static Type *getIndexedTypeInternal(Type *Ptr, ArrayRef<IndexTy> IdxList) {</div>
<div>+ if (Ptr->isVectorTy()) {</div><div>+ assert(1 == IdxList.size() &&</div><div>+ "GEP with vector pointers must have a single index");</div></div><div><br></div><div>Please use "IdxList.size() == 1 &&". I appreciate why you're putting the constant first, but LLVM just doesn't use this style.</div>
<div><br></div><div><div>@@ -2652,9 +2674,15 @@</div><div> return SrcTy->isFPOrFPVectorTy() && DstTy->isIntOrIntVectorTy() &&</div><div> SrcLength == DstLength;</div><div> case Instruction::PtrToInt:</div>
<div>- return SrcTy->isPointerTy() && DstTy->isIntegerTy();</div><div>+ if (SrcTy->isVectorTy() != DstTy->isVectorTy())</div><div>+ return false;</div><div>+ return SrcTy->getScalarType()->isPointerTy() &&</div>
<div>+ DstTy->getScalarType()->isIntegerTy();</div><div> case Instruction::IntToPtr:</div><div>- return SrcTy->isIntegerTy() && DstTy->isPointerTy();</div><div>+ if (SrcTy->isVectorTy() != DstTy->isVectorTy())</div>
<div>+ return false;</div><div>+ return SrcTy->getScalarType()->isIntegerTy() &&</div><div>+ DstTy->getScalarType()->isPointerTy();</div></div><div><br></div><div>That's not enough; for vectors you'll need to check the number of elements too. You could refactor getNumElements() up to the Type base class and then use it here? (The verifier rejects it, and should continue to do so on top of adding it here so that the .ll parser produces a better error, etc.)</div>
<div><br></div><div>Testcase:</div><div><br></div><div> ; RUN: llvm-as %s -disable-output</div><div><div> @G = constant <3 x i64> ptrtoint (<2 x i8*> <i8* null, i8* null> to <3 x i64>)</div></div>
<div><br></div><div>It throws an assertion. To see what it should do, try s/ptrtoint/inttoptr/ on the testcase and look at the error.</div><div><br></div><div><div> void Verifier::visitGetElementPtrInst(GetElementPtrInst &GEP) {</div>
<div>- Assert1(cast<PointerType>(GEP.getOperand(0)->getType())</div><div>- ->getElementType()->isSized(),</div><div>+ Type *TargetTy = GEP.getOperand(0)->getType();</div></div><div><br></div>
<div>"Type *TargetTy = GEP.getPointerOperandType();"? Or "GEP.getPointerOperand()->getType();"?</div><div><br></div><div>Speaking of which, what do you plan to do about GetElementPtrInst::getPointerOperandType:</div>
<div><br></div><div><div> /// getPointerOperandType - Method to return the pointer operand as a</div><div> /// PointerType.</div><div> PointerType *getPointerOperandType() const {</div><div> return reinterpret_cast<PointerType*>(getPointerOperand()->getType());</div>
<div> }</div></div><div><br></div><div>I don't understand why that was ever reinterpret_cast<> instead of just cast<> for starters. That function can either be removed entirely or changed to return Type*, and all of its callers need to be audited to support the fact that the pointer operand might now be a vector. (Don't forget to also fix the copy in include/llvm/Operator.h.)</div>
<div><br></div><div>lib/Analysis/InstructionSimplify.cpp's llvm::SimplifyGEPInst needs to be fixed to handle vectors. Testcase:</div><div><br></div><div><div> // RUN: opt -instsimplify %s -disable-output</div><div><div>
declare void @helper(<2 x i8*>)</div><div> define void @test(<2 x i8*> %a) {</div><div> %A = getelementptr <2 x i8*> %a, <2 x i32> <i32 0, i32 0></div><div> call void @helper(<2 x i8*> %A)</div>
<div> ret void</div><div> }</div></div></div><div><br></div><div>llvm::GetPointerBaseWithConstantOffset lib/Analysis/ValueTracking.cpp will crash in this code:</div><div><br></div><div><div> gep_type_iterator GTI = gep_type_begin(GEP);</div>
<div> for (User::op_iterator I = GEP->idx_begin(), E = GEP->idx_end(); I != E;</div><div> ++I, ++GTI) {</div><div> ConstantInt *OpC = cast<ConstantInt>(*I);</div></div><div><br></div><div>My attempts to write a testcase were blocked by the problem above (GVN calls into instsimplify), but even with that fixed it may be possible to manifest through opt -gvn. (If it isn't now, it certainly will be once loads and stores support vector pointer operands.)</div>
<div><br></div><div>lib/Analysis/ValueTracking.cpp's ComputeMaskedBits has this assert:</div><div><br></div><div><div> assert((V->getType()->isIntOrIntVectorTy() || V->getType()->isPointerTy())</div><div>
&& "Not integer or pointer type!");</div></div><div><br></div><div>and it should be extended to support vectors of pointers. Similarly with lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp. Testcase:</div>
<div><br></div><div><div><div> ; RUN: opt -instcombine %s -disable-output</div><div> target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"</div>
<div> target triple = "x86_64-unknown-linux-gnu"</div><div> define <2 x i1> @test(<2 x i8*> %a, <2 x i8*> %b) {</div><div> %A = icmp eq <2 x i8*> %a, %b</div><div> ret <2 x i1> %A</div>
<div> }</div></div></div><div><br></div><div>Scanning the code briefly, I think just adjusting the assert will be sufficient.</div><div><br></div><div>There's still a problem with the parsing of constants. This:</div>
<div><br></div><div><div><div> ; RUN: llvm-as -disable-output %s</div><div> @G1 = global i8 zeroinitializer</div></div><div> @g = constant <2 x i8*> getelementptr (<2 x i8*> <i8* @G1, i8* @G1>, <2 x i32> <i32 0, i32 0>)</div>
</div><div><br></div><div>doesn't parse, and I think it should. Please fix!</div><div><br></div><div>All the rules you changed for instructions need to be fixed for constants as well. Consider ConstantExpr::getIntToPtr:</div>
<div><br></div><div><div> Constant *ConstantExpr::getIntToPtr(Constant *C, Type *DstTy) {</div><div> assert(C->getType()->isIntegerTy() && "IntToPtr source must be integral");</div><div> assert(DstTy->isPointerTy() && "IntToPtr destination must be a pointer");</div>
<div> return getFoldedCast(Instruction::IntToPtr, C, DstTy);</div><div> }</div></div><div><br></div><div>Testcase:</div><div><br></div><div><div> ; RUN: opt -instcombine %s -disable-output</div><div> define <2 x i1> @test(<2 x i8*> %a) {</div>
<div> %A = inttoptr <2 x i32> <i32 1, i32 2> to <2 x i8*></div><div> %B = icmp ult <2 x i8*> %A, zeroinitializer</div><div> ret <2 x i1> %B</div><div> }</div></div><div><br></div><div>
I'm going to let you do an audit for ICmp inside lib/Transforms/InstCombine/InstCombineComapres.cpp. The good news is that all this code should already work for ints, vec-of-int and pointers, so adding vec-of-ptr shouldn't actually be that crazy. This file has all sorts of great stuff like "If we can optimize a 'icmp GEP, P' or 'icmp P, GEP', do so now." :-)</div>
<div><br></div><div>Unit tests:</div><div> * please write a test which demonstrates that Value::stripPointerCasts peers through casts of vectors of pointers (or at the very least, doesn't crash).</div><div> * please write one for the fix to llvm::GetPointerBaseWithConstantOffset.</div>
<div> * I wasn't able to test llvm::ConstantFoldCompareInstOperands which I expect to crash when given "icmp (ptrtoint <2 x i8*> x), 0". Please add a test for this, either in .ll or as a C++ unittest.</div>
<div><br></div><div>Finally, I feel like I should mention a few things I'm deliberately ignoring: the interpreter, the C backend and the CPP backend. I consider the first two ripe to be ripped out, and the last one isn't really kept up to date anyways.</div>
<div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Nadav<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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:<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>] <br>
<b>Sent:</b> Wednesday, November 30, 2011 00:57</span></p><div><div><br><b>To:</b> Rotem, Nadav<br><b>Cc:</b> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b> Re: Updated pointer-vector patch<u></u><u></u></div>
</div><p></p></div><div><div><p class="MsoNormal"><u></u> <u></u></p><div><p class="MsoNormal">On 29 November 2011 10:27, Rotem, Nadav <<a href="mailto:nadav.rotem@intel.com" target="_blank">nadav.rotem@intel.com</a>> wrote:<u></u><u></u></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.<u></u><u></u></p><div><p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
</div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">One high-level question. Are you planning to permit:<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><div>
<p class="MsoNormal">
%X = load <4 x i8*> %ptr<u></u><u></u></p></div></div><div><p class="MsoNormal"><u></u> <u></u></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).<u></u><u></u></p>
</div><div><p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p></blockquote><div><p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
</div><div><p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
</blockquote><div><p class="MsoNormal"><u></u> <u></u></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. :)<u></u><u></u></p>
</div><div><p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
</blockquote><div><p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
</div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">Here's a testcase:<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal"> ; RUN: opt -instcombine -S -o - %s | llvm-as<u></u><u></u></p>
</div><div><div><p class="MsoNormal"> ; RUN: opt -instcombine -globalopt -S -o - %s | llvm-as<u></u><u></u></p></div></div><div><p class="MsoNormal"> @G1 = global i32 zeroinitializer<u></u><u></u></p></div><div><p class="MsoNormal">
@G2 = global i32 zeroinitializer<u></u><u></u></p></div><div><p class="MsoNormal"> @g = global <2 x i32*> zeroinitializer<u></u><u></u></p></div><div><p class="MsoNormal"> %0 = type { i32, void ()* }<u></u><u></u></p>
</div><div><p class="MsoNormal"> @llvm.global_ctors = appending global [1 x %0] [%0 { i32 65535, void ()* @test }]<u></u><u></u></p></div><div><p class="MsoNormal"> define internal void @test() {<u></u><u></u></p></div>
<div><div><p class="MsoNormal"> %A = insertelement <2 x i32*> undef, i32* @G1, i32 0<u></u><u></u></p></div><div><p class="MsoNormal"> %B = insertelement <2 x i32*> %A, i32* @G2, i32 1<u></u><u></u></p>
</div></div><div><p class="MsoNormal"> store <2 x i32*> %B, <2 x i32*>* @g<u></u><u></u></p></div><div><div><p class="MsoNormal"> ret void<u></u><u></u></p></div><div><p class="MsoNormal"> }<u></u><u></u></p>
</div></div><div><p class="MsoNormal"><u></u> <u></u></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 ?<u></u><u></u></p>
</blockquote><div><p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
</div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">Nick<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></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<u></u><u></u></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" target="_blank">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" target="_blank">llvm-commits@cs.uiuc.edu</a><br>Subject: Re: Updated pointer-vector patch<u></u><u></u></p></div>
<div><div><p class="MsoNormal">On 28 November 2011 12:33, Rotem, Nadav <<a href="mailto:nadav.rotem@intel.com" target="_blank">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.<u></u><u></u></p></div></div></blockquote></div><p class="MsoNormal"><u></u> <u></u></p></div></div></div><div><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></div></div></div></blockquote></div><br>