[llvm-commits] Updated pointer-vector patch

Rotem, Nadav nadav.rotem at intel.com
Tue Nov 29 10:27:22 PST 2011

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. 

What I did not fix:

1. I did not add optimizations to ConstantFold. I plan to add these in the future.
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.
3. Did not add support to the ASM parser for constant pointer vectors. I may add this feature in the future. 
4. I did not add unit tests. 	Was this related to the constant  ?

I attached the revised patch. 


-----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,
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

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

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.


> 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 --------------
A non-text attachment was scrubbed...
Name: vector_gep.patch
Type: application/octet-stream
Size: 31505 bytes
Desc: vector_gep.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111129/8597169e/attachment.obj>

More information about the llvm-commits mailing list