[llvm-commits] Updated pointer-vector patch

[Nick Lewycky]

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.