[llvm-commits] Updated pointer-vector patch
Nick Lewycky
nlewycky at google.com
Tue Nov 29 14:56:35 PST 2011
On 29 November 2011 10:27, Rotem, Nadav <nadav.rotem at intel.com> wrote:
> Hi Nick,
>
> Thank you for reviewing my code. I made corrections, and prepared a new
> version. I made the following changes:
>
> 1. I added support for icmp of pointer-vectors.
> 2. Made corrections in LangRef (inbound, summary, inttoptr example, etc).
> 3. Added additional verifications (vector width, etc)
> 4. Changed the casts to dyn_casts.
> 5. Simplified the verification code that you said was confusing.
> 6. Documented getBitWidth.
> 7. I went over the users of getIndexedOffset, and changed some of them to
> abort the optimization if a pointer-vector is used.
> 8. Fixed IndVarSimplify.
>
Excellent, thanks! Unfortunately I'm rather busy today and am unlikely to
be able to finish reviewing it before tomorrow. Sorry.
One high-level question. Are you planning to permit:
%X = load <4 x i8*> %ptr
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).
What I did not fix:
>
> 1. I did not add optimizations to ConstantFold. I plan to add these in the
> future.
>
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.
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.
>
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. :)
3. Did not add support to the ASM parser for constant pointer vectors. I
> may add this feature in the future.
>
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.
Here's a testcase:
; RUN: opt -instcombine -S -o - %s | llvm-as
; RUN: opt -instcombine -globalopt -S -o - %s | llvm-as
@G1 = global i32 zeroinitializer
@G2 = global i32 zeroinitializer
@g = global <2 x i32*> zeroinitializer
%0 = type { i32, void ()* }
@llvm.global_ctors = appending global [1 x %0] [%0 { i32 65535, void ()*
@test }]
define internal void @test() {
%A = insertelement <2 x i32*> undef, i32* @G1, i32 0
%B = insertelement <2 x i32*> %A, i32* @G2, i32 1
store <2 x i32*> %B, <2 x i32*>* @g
ret void
}
4. I did not add unit tests. Was this related to the constant ?
>
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.
Nick
> I attached the revised patch.
>
> Thanks,
> Nadav
>
>
> -----Original Message-----
> From: Nick Lewycky [mailto:nlewycky at google.com]
> Sent: Tuesday, November 29, 2011 04:39
> To: Rotem, Nadav
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: Updated pointer-vector patch
>
> On 28 November 2011 12:33, Rotem, Nadav <nadav.rotem at intel.com> wrote:
> >
> > I attached the updated pointer-vector patch, following comments from
> Nick Lewycky.
>
> To elaborate, the comment was that the only pointer types we should
> allow are those which would let us load the vector and produce a legal
> vector type.
>
> +<p>In cases where the pointer argument is a vector of pointers, only a
> + single index may be used. For example: </p>
> +<pre class="doc_code">
> + %A = getelementptr <4 x i8*> %ptrs, <4 x i64> %offsets,
> +</pre>
> Also mention that the number of elements must be the same.
> - %X = ptrtoint i32* %X to i8 <i>; yields truncation on
> 32-bit architecture</i>
> - %Y = ptrtoint i32* %x to i64 <i>; yields zero extension on
> 32-bit architecture</i>
> + %X = ptrtoint i32* %X to i8 <i>; yields
> truncation on 32-bit architecture</i>
>
> While you're here, please make this use "%x" (lowercase) as the
> operand consistently. The existing code "%X = op %X" is bad form
> (using its own result).
>
> Also update the overview and summary for the instruction, replace
> "pointer" with "pointer or vector of pointers" and "integer" with
> "integer or vector of integers". See the documentation of "add" for
> example. The semantics can simply mention that for a vector it does
> the operation elementwise.
>
> Same with inttoptr and bitcast.
>
> template <typename IndexTy>
> static Type *getIndexedTypeInternal(Type *Ptr, ArrayRef<IndexTy> IdxList)
> {
> + if (Ptr->isVectorTy()) {
> + IndexTy Index = IdxList[0];
> "Index" is a dead variable right here. Did you mean to use it?
> +unsigned GetElementPtrInst::getAddressSpace(Value *Ptr) {
> + Type* Ty = Ptr->getType();
> "Type* Ty" --> "Type *Ty".
> + if (Ty->isVectorTy())
> + Ty = cast<VectorType>(Ty)->getElementType();
> +
> + if (Ty->isPointerTy())
> + return cast<PointerType>(Ty)->getAddressSpace();
>
> Please use "if (VectorType *VTy = dyn_cast<VectorType>(Ty)) Ty =
> VTy->getElementTy();". The difference is that when compiled in debug
> mode, the code you wrote will do two tests.
>
> + Assert1(SrcTy->isVectorTy() == DestTy->isVectorTy(),
> + "PtrToInt type mismatch", &I);
>
> Please also verify the number of elements. Again in IntToPtr.
>
> Type *ElTy =
> GetElementPtrInst::getIndexedType(GEP.getOperand(0)->getType(), Idxs);
> Assert1(ElTy, "Invalid indices for GEP pointer type!", &GEP);
> - Assert2(GEP.getType()->isPointerTy() &&
> + if (GEP.getOperand(0)->getType()->isPointerTy()) {
> + Assert2(GEP.getType()->isPointerTy() &&
> cast<PointerType>(GEP.getType())->getElementType() == ElTy,
> "GEP is not of right type for indices!", &GEP, ElTy);
> + } else {
> + Assert1(Idxs.size() == 1, "Invalid number of indices!", &GEP);
>
> I'm confused. Can this assert ever fire when "Invalid indices for GEP
> pointer type!" doesn't?
>
> bool VectorType::isValidElementType(Type *ElemTy) {
> + if (ElemTy->isPointerTy())
> + ElemTy = cast<PointerType>(ElemTy)->getElementType();
> return ElemTy->isIntegerTy() || ElemTy->isFloatingPointTy();
> }
>
> Again, please use dyn_cast.
>
>
> The changes in this patch fail to address vectors of pointers in an
> icmp operation. It's fine if you want to leave that illegal and add
> support for it later. The verifier will reject vectors of pointers as
> icmp arguments, while continuing to accept integer vectors and scalar
> pointers.
>
> The change to the LangRef doesn't address the meaning of inbounds on a
> vector-gep. It seems reasonable to say that it applies to each of the
> computations element-wise.
>
> You need to update the comment on VectorType::getBitWidth to indicate
> that it will return zero on vectors of pointers.
>
> You need to update TargetData::getIndexedOffset to handle a vector
> index, or update all of its callers. I don't know how to do this
> because it returns a single integer, but you really want something
> element-wise.
>
> Currently SROA will bail on "alloca <4 x i8*>". It (and mem2reg) need
> to be taught to handle them. This is important for performance, but
> can hold off until later.
>
> This code in ConstantFold.cpp:
>
> // If the right hand side is a bitcast, try using its inverse to simplify
> // it by moving it to the left hand side. We can't do this if it would turn
> // a vector compare into a scalar compare or visa versa.
> if (ConstantExpr *CE2 = dyn_cast<ConstantExpr>(C2)) {
> Constant *CE2Op0 = CE2->getOperand(0);
> if (CE2->getOpcode() == Instruction::BitCast &&
> CE2->getType()->isVectorTy() == CE2Op0->getType()->isVectorTy()) {
> Constant *Inverse = ConstantExpr::getBitCast(C1, CE2Op0->getType());
> return ConstantExpr::getICmp(pred, Inverse, CE2Op0);
> }
> }
>
> assumes that it can produce a bitcast/icmp of any vector you can
> bitcast. Now that we permit bitcasting on vectors of pointers, that is
> no longer true. Please write a testcase, or let me know if you can't.
>
> The ASM parser doesn't parse constant vectors of pointers. Testcase:
>
> // llvm-as < %s -disable-output
> @G1 = global i32 zeroinitializer
> @G2 = global i32 zeroinitializer
> @g = constant <2 x i32*> <i32* @G1, i32* @G2>
>
> FoldBitCast in ConstantFolding.cpp should be taught that it can create
> vectors of pointers too:
>
> // If this is a scalar -> vector cast, convert the input into a <1 x
> scalar>
> // vector so the code below can handle it uniformly.
> if (isa<ConstantFP>(C) || isa<ConstantInt>(C)) {
>
> || C->getType() is pointer to a vector argument.
>
> Constant *Ops = C; // don't take the address of C!
> return FoldBitCast(ConstantVector::get(Ops), DestTy, TD);
> }
>
> You should add tests for the basic correctness to tests under
> unittests/. Show that creating a vector of null pointers becomes a
> constant aggregate zero, show that you can create a GEP with a
> non-pointer argument (the vector).
>
> This code inside IndVarSimplify.cpp needs to change
>
> // Retrieve the pointer operand of the GEP. Don't use GetUnderlyingObject
> // because it understands lcssa phis while SCEV does not.
> Value *FromPtr = FromVal;
> Value *ToPtr = ToVal;
> if (GEPOperator *GEP = dyn_cast<GEPOperator>(FromVal)) {
> FromPtr = GEP->getPointerOperand();
> }
> if (GEPOperator *GEP = dyn_cast<GEPOperator>(ToVal)) {
> ToPtr = GEP->getPointerOperand();
> }
>
> because it sends the FromPtr and ToPtr to SCEV:
>
> const SCEV *FromBase = SE->getPointerBase(SE->getSCEV(FromPtr));
> const SCEV *ToBase = SE->getPointerBase(SE->getSCEV(ToPtr));
>
> which does not support vectors of pointers, only normal pointers.
>
> Nick
>
> >
> > Thanks,
> > Nadav
> >
> > ---------------------------------------------------------------------
> > Intel Israel (74) Limited
> >
> > This e-mail and any attachments may contain confidential material for
> > the sole use of the intended recipient(s). Any review or distribution
> > by others is strictly prohibited. If you are not the intended
> > recipient, please contact the sender and delete all copies.
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111129/15105c66/attachment.html>
More information about the llvm-commits
mailing list