[llvm-commits] Updated pointer-vector patch

[Rotem, Nadav]

Hi Nick,

I fixed #3  (ASM parser for constant pointer vectors) and added the testcase that you provided.

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.

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.

Nadav


From: Nick Lewycky [mailto:nlewycky at google.com]
Sent: Wednesday, November 30, 2011 00:57
To: Rotem, Nadav
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: Updated pointer-vector patch

On 29 November 2011 10:27, Rotem, Nadav <nadav.rotem at intel.com<mailto: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<mailto:nlewycky at google.com>]
Sent: Tuesday, November 29, 2011 04:39
To: Rotem, Nadav
Cc: llvm-commits at cs.uiuc.edu<mailto: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<mailto: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.

---------------------------------------------------------------------
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/20111130/faa2c6cd/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vector_gep.patch
Type: application/octet-stream
Size: 33167 bytes
Desc: vector_gep.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111130/faa2c6cd/attachment.obj>