[llvm-commits] Patch to add address space support to getIntPtrTy

[Eli Friedman]

On Fri, Oct 19, 2012 at 4:17 PM, Villmow, Micah <Micah.Villmow at amd.com> wrote:
> Thanks for the review Eli, new patch attached and enjoy the weekend.

@@ -178,9 +186,14 @@
     // Check if this has the right signature.
     if (FT->getNumParams() != 3 ||
         FT->getReturnType() != FT->getParamType(0) ||
-        FT->getParamType(0) != FT->getParamType(1) ||
-        FT->getParamType(0) != Type::getInt8PtrTy(Context) ||
-        FT->getParamType(2) != TD->getIntPtrType(Context))
+        FT->getParamType(0) != FT->getParamType(1))
+      return 0;
+
+    assert(FT->getParamType(0)->isPointerTy() &&
+        "Must be a pointer type.");

Nothing in the code guarantees this assertion.

Index: lib/Transforms/Utils/SimplifyLibCalls.cpp
===================================================================
--- lib/Transforms/Utils/SimplifyLibCalls.cpp	(revision 166296)
+++ lib/Transforms/Utils/SimplifyLibCalls.cpp	(working copy)
@@ -102,14 +102,17 @@
   virtual Value *callOptimizer(Function *Callee, CallInst *CI,
IRBuilder<> &B) {
     this->CI = CI;
     FunctionType *FT = Callee->getFunctionType();
-    LLVMContext &Context = CI->getParent()->getContext();

     // Check if this has the right signature.
     if (FT->getNumParams() != 4 || FT->getReturnType() !=
FT->getParamType(0) ||
         !FT->getParamType(0)->isPointerTy() ||
-        !FT->getParamType(1)->isPointerTy() ||
-        FT->getParamType(2) != TD->getIntPtrType(Context) ||
-        FT->getParamType(3) != TD->getIntPtrType(Context))
+        !FT->getParamType(1)->isPointerTy())
+      return 0;
+
+    Type *PT0 = FT->getParamType(0);
+    Type *PT1 = FT->getParamType(1);
+    if (FT->getParamType(2) != TD->getIntPtrType(PT0) ||
+        FT->getParamType(3) != TD->getIntPtrType(PT1))
       return 0;

     if (isFoldable(3, 2, false)) {

You don't need to reorganize the code here.  (Same for other places in
SimplifyLibCalls.)

Index: lib/Analysis/Lint.cpp
===================================================================
--- lib/Analysis/Lint.cpp	(revision 166296)
+++ lib/Analysis/Lint.cpp	(working copy)
@@ -606,6 +606,9 @@
   // TODO: Look through eliminable cast pairs.
   // TODO: Look through calls with unique return values.
   // TODO: Look through vector insert/extract/shuffle.
+  // FIXME: Not sure how to get the correct address space here,
+  //  so will just set it to zero for now.
+  unsigned AS = 0;

There's only one place in this function that uses AS, and it should be
pretty straightforward to get the appropriate type there.

Index: lib/Target/Hexagon/HexagonISelLowering.cpp
===================================================================
--- lib/Target/Hexagon/HexagonISelLowering.cpp	(revision 166296)
+++ lib/Target/Hexagon/HexagonISelLowering.cpp	(working copy)
@@ -1017,7 +1017,7 @@
   Result = DAG.getTargetGlobalAddress(GV, dl, getPointerTy(), Offset);

   HexagonTargetObjectFile &TLOF =
-    (HexagonTargetObjectFile&)getObjFileLowering();
+    (HexagonTargetObjectFile&)(getObjFileLowering());
   if (TLOF.IsGlobalInSmallSection(GV, getTargetMachine())) {
     return DAG.getNode(HexagonISD::CONST32_GP, dl, getPointerTy(), Result);
   }

??

+/// getIntPtrType - Return an integer type that is the same size or
+/// greater to the pointer size of the specific PointerType.
+IntegerType *DataLayout::getIntPtrType(Type *Ty) const {
+  LLVMContext &C = Ty->getContext();
+  // For pointers, we return the size for the specific address space.
+  if (Ty->isPointerTy()) return IntegerType::get(C, getTypeSizeInBits(Ty));
+  // For vector of pointers, we return the size of the address space
+  // of the pointer type.
+  if (Ty->isVectorTy() &&
cast<VectorType>(Ty)->getElementType()->isPointerTy())
+    return IntegerType::get(C,
+        getTypeSizeInBits(cast<VectorType>(Ty)->getElementType()));
+  // Otherwise return the address space for the default address space.
+  return getIntPtrType(C, 0);
+}

Sorry to bug you again about this, but can you give a specific example
in the patch where the "Otherwise" case is useful?

@@ -175,7 +174,8 @@
     // Translate this as an integer zero so that it can be
     // local-CSE'd with actual integer zeros.
     Reg =
-      getRegForValue(Constant::getNullValue(TD.getIntPtrType(V->getContext())));
+      getRegForValue(Constant::getNullValue(
+            TD.getIntPtrType(V->getType())));

Nit: this fits on a single line.

@@ -2904,8 +2907,9 @@
       } else {
         // LLVM does not support vectors of pointers, so turn any pointers into
         // integers.
-        if (isa<PointerType>(Elt->getType()))
-          Elt = Builder.CreatePtrToInt(Elt, TD.getIntPtrType(Context));
+        if (isa<PointerType>(Elt->getType())) {
+          Elt = Builder.CreatePtrToInt(Elt, TD.getIntPtrType(Elt->getType()));
+        }

Nit: unnecessary braces.

Otherwise, this is looking good.

-Eli