<div class="gmail_quote">On 29 November 2011 10:27, Rotem, Nadav <span dir="ltr"><<a href="mailto:nadav.rotem@intel.com">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;">
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.<br></blockquote><div><br></div><div>Excellent, thanks! Unfortunately I'm rather busy today and am unlikely to be able to finish reviewing it before tomorrow. Sorry.</div><div><br></div><div>One high-level question. Are you planning to permit:</div>
<div><br></div><div><div> %X = load <4 x i8*> %ptr</div></div><div><br></div><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">What I did not fix:<br>
<br>
1. I did not add optimizations to ConstantFold. I plan to add these in the future.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
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.<br></blockquote><div><br></div><div>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. :)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
3. Did not add support to the ASM parser for constant pointer vectors. I may add this feature in the future.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>Here's a testcase:</div><div><br></div><div> ; RUN: opt -instcombine -S -o - %s | llvm-as</div><div><div> ; RUN: opt -instcombine -globalopt -S -o - %s | llvm-as</div></div><div> @G1 = global i32 zeroinitializer</div>
<div> @G2 = global i32 zeroinitializer</div><div> @g = global <2 x i32*> zeroinitializer</div><div> %0 = type { i32, void ()* }</div><div> @llvm.global_ctors = appending global [1 x %0] [%0 { i32 65535, void ()* @test }]</div>
<div> define internal void @test() {</div><div><div> %A = insertelement <2 x i32*> undef, i32* @G1, i32 0</div><div> %B = insertelement <2 x i32*> %A, i32* @G2, i32 1</div></div><div> store <2 x i32*> %B, <2 x i32*>* @g</div>
<div><div> ret void</div><div> }</div><div></div></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">4. I did not add unit tests. Was this related to the constant ?<br>
</blockquote><div><br></div><div>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.</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;">
<br>
I attached the revised patch.<br>
<br>
Thanks,<br>
Nadav<br>
<div class="im HOEnZb"><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<br>
<br>
</div><div class="HOEnZb"><div class="h5">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.<br>
</div></div></blockquote></div><br>