llvm dev - Jul 2008 - [LLVMdev] GEP::getIndexValid() with other iterators

Hi all,

currently, GetElementPtrInst has a method getIndexedType, which has a
templated variant. You pass in a begin and an end iterator, and it will find
the indexed type for those. However, both iterators must iterate over Value*.

For some argpromotion code, I would like to pass in iterators that iterate
over unsigneds instead of Value*. I currently solve this by transforming my
vector<unsigned> into a vector<Value*>, but this is hardly an
efficient
solution.

Currently, however, there is already a templated version of getIndexedType,
probably so it can iterate over any pointer. In there, the begin pointer is
cast as follows:
 (const Value*)&*IdxBegin

Though this works, this has the nasty side effect of accepting a lot of
iterator types that are not really supported. In particular, when I passed in
an iterator over unsigned, the compiler would happily cast the resulting
unsigned* to a const Value*, resulting in all kinds of badness at runtime (and
not even a warning at compile time).

However, simply removing this cast seems to work for me, so I suppose there is
some compiler out there that will warn about this? Or is the cast really not
needed (anymore)?

Anyway, when one removes this cast and also makes the other getIndexedType
implementation (which is called by this one) a template, passing in an
iterator over unsigned should also work. The actual types over which an
iterator is allowed should now be limited by the parameters to
CompositeType::indexValid() and CompositeType::getTypeAtIndex() (which
currently are Value* or unsigned).

I've attached a patch which does exactly this. Since my template-fu is not
so
strong, please review :-) I've also attached another patch to argpromotion,
which passes an unsigned iterator to getIndexedType() 
(look for the comment
	// This uses the new templated getIndexdType
in the code)

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: argpromotion.diff
Type: text/x-diff
Size: 24647 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080716/b315b259/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: getindexedtype.diff
Type: text/x-diff
Size: 3738 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080716/b315b259/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080716/b315b259/attachment.sig>

Hi all,

once more with the patch inline for easy review. I did not include the
argpromotion pass here, since it's not the main topic of this post.

Gr.

Matthijs

Index: lib/VMCore/Instructions.cpp
==================================================================---
lib/VMCore/Instructions.cpp	(revision 53672)
+++ lib/VMCore/Instructions.cpp	(working copy)
@@ -1068,41 +1068,6 @@
   init(Ptr, Idx, Name);
 }
 
-// getIndexedType - Returns the type of the element that would be loaded with
-// a load instruction with the specified parameters.
-//
-// A null type is returned if the indices are invalid for the specified
-// pointer type.
-//
-const Type* GetElementPtrInst::getIndexedType(const Type *Ptr,
-                                              Value* const *Idxs,
-                                              unsigned NumIdx) {
-  const PointerType *PTy = dyn_cast<PointerType>(Ptr);
-  if (!PTy) return 0;   // Type isn't a pointer type!
-  const Type *Agg = PTy->getElementType();
-
-  // Handle the special case of the empty set index set...
-  if (NumIdx == 0)
-    return Agg;
-
-  unsigned CurIdx = 1;
-  for (; CurIdx != NumIdx; ++CurIdx) {
-    const CompositeType *CT = dyn_cast<CompositeType>(Agg);
-    if (!CT || isa<PointerType>(CT)) return 0;
-    Value *Index = Idxs[CurIdx];
-    if (!CT->indexValid(Index)) return 0;
-    Agg = CT->getTypeAtIndex(Index);
-
-    // If the new type forwards to another type, then it is in the middle
-    // of being refined to another type (and hence, may have dropped all
-    // references to what it was using before).  So, use the new forwarded
-    // type.
-    if (const Type *Ty = Agg->getForwardedType())
-      Agg = Ty;
-  }
-  return CurIdx == NumIdx ? Agg : 0;
-}
-
 const Type* GetElementPtrInst::getIndexedType(const Type *Ptr, Value *Idx) {
   const PointerType *PTy = dyn_cast<PointerType>(Ptr);
   if (!PTy) return 0;   // Type isn't a pointer type!
Index: include/llvm/Instructions.h
==================================================================---
include/llvm/Instructions.h	(revision 53672)
+++ include/llvm/Instructions.h	(working copy)
@@ -407,9 +407,35 @@
   /// Null is returned if the indices are invalid for the specified
   /// pointer type.
   ///
+  template <typename IndexTy>
   static const Type *getIndexedType(const Type *Ptr,
-                                    Value* const *Idx, unsigned NumIdx);
+                                    IndexTy const *Idxs, unsigned NumIdx) {
+    const PointerType *PTy = dyn_cast<PointerType>(Ptr);
+    if (!PTy) return 0;   // Type isn't a pointer type!
+    const Type *Agg = PTy->getElementType();
 
+    // Handle the special case of the empty set index set...
+    if (NumIdx == 0)
+      return Agg;
+
+    unsigned CurIdx = 1;
+    for (; CurIdx != NumIdx; ++CurIdx) {
+      const CompositeType *CT = dyn_cast<CompositeType>(Agg);
+      if (!CT || isa<PointerType>(CT)) return 0;
+      IndexTy Index = Idxs[CurIdx];
+      if (!CT->indexValid(Index)) return 0;
+      Agg = CT->getTypeAtIndex(Index);
+
+      // If the new type forwards to another type, then it is in the middle
+      // of being refined to another type (and hence, may have dropped all
+      // references to what it was using before).  So, use the new forwarded
+      // type.
+      if (const Type *Ty = Agg->getForwardedType())
+        Agg = Ty;
+    }
+    return CurIdx == NumIdx ? Agg : 0;
+  }
+
   template<typename InputIterator>
   static const Type *getIndexedType(const Type *Ptr,
                                     InputIterator IdxBegin, 
@@ -422,7 +448,7 @@
 
     if (NumIdx 0)
       // This requires that the iterator points to contiguous memory.
-      return getIndexedType(Ptr, (Value *const *)&*IdxBegin, NumIdx);
+      return getIndexedType(Ptr, &*IdxBegin, NumIdx);
     else
       return getIndexedType(Ptr, (Value *const*)0, NumIdx);
   }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080716/0b844a2d/attachment.sig>

Hi all,

no comments on this patch? I haven't seen any problems so far, so I'll
be
committing this tomorrow.

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080721/68964912/attachment.sig>

On Jul 16, 2008, at 9:58 AM, Matthijs Kooijman wrote:
> Hi all,
>
> once more with the patch inline for easy review. I did not include the
> argpromotion pass here, since it's not the main topic of this post.
Hi Matthijs,

I'd prefer to not turn this into a template.  Why not just define a  
version that takes an array of uint64_t's or something like that?

-Chris