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

[Villmow, Micah]

Thanks for the review Eli, new patch attached and enjoy the weekend.

Micah
___________________________________
From: Eli Friedman [eli.friedman at gmail.com]
Sent: Friday, October 19, 2012 5:20 PM
To: Villmow, Micah
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] Patch to add address space support to getIntPtrTy

On Fri, Oct 19, 2012 at 2:57 PM, Villmow, Micah <Micah.Villmow at amd.com> wrote:
> Eli,
>  I've made the requested modifications and this patch looks a lot cleaner with the new API changes.

+  /// getIntPtrType - Return an unsigned integer type that is the same size or
+  /// greater to the pointer size based on the Type.

Drop "unsigned"; LLVM IR integers don't have a sign.

+  // Build a mask for high order bits.
+  unsigned AS = cast<GEPOperator>(GEP)->getPointerAddressSpace();
   gep_type_iterator GTI = gep_type_begin(GEP);
-  Type *IntPtrTy = TD.getIntPtrType(GEP->getContext());
+  Type *IntPtrTy = TD.getIntPtrType(GEP->getContext(), AS);
   Value *Result = Constant::getNullValue(IntPtrTy);

The "build a mask" comment shouldn't move.

+  // 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()));

If someone called getIntPtrType with a vector, they would probably
expect a vector result type.

+  // Otherwise return the address space for the default address space.
+  return getIntPtrType(C, 0);

I would prefer an assertion; there isn't any reasonable default here.

   // No-op casts are trivially coalesced by fast-isel.
-  if (const CastInst *Cast = dyn_cast<CastInst>(I))
-    if (Cast->isNoopCast(TD.getIntPtrType(Cast->getContext())) &&
+  if (const CastInst *Cast = dyn_cast<CastInst>(I)) {
+    if (Cast->isNoopCast(TD) &&
         !hasTrivialKill(Cast->getOperand(0)))
       return false;
+    }

Indentation?

Index: lib/Transforms/Scalar/SimplifyLibCalls.cpp
===================================================================
--- lib/Transforms/Scalar/SimplifyLibCalls.cpp  (revision 166296)
+++ lib/Transforms/Scalar/SimplifyLibCalls.cpp  (working copy)
@@ -135,7 +135,7 @@

 namespace {
 //===---------------------------------------===//
-// 'stpcpy' Optimizations
+// 'strcpy' Optimizations

 struct StpCpyOpt: public LibCallOptimization {
   bool OptChkCall;  // True if it's optimizing a __stpcpy_chk libcall.

Wrong.

@@ -1057,11 +1057,13 @@

     // Converting from one pointer type to another or between a pointer and an
     // integer of the same size is safe even if we do not have a body.
+    // FIXME: Not sure what to do here, so setting AS to 0.
+    // How can the AS for a function call be outside the default?

Outdated comment?

Index: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp   (revision 166296)
+++ lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp   (working copy)
@@ -173,7 +173,7 @@
   // Ensure that the alloca array size argument has type intptr_t, so that
   // any casting is exposed early.
   if (TD) {
-    Type *IntPtrTy = TD->getIntPtrType(AI.getContext());
+    Type *IntPtrTy = TD->getIntPtrType(AI.getContext(), 0);
     if (AI.getArraySize()->getType() != IntPtrTy) {
       Value *V = Builder->CreateIntCast(AI.getArraySize(),
                                         IntPtrTy, false);

Might as well use getIntPtrType(AI.getType()) .

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;
   V = OffsetOk ? GetUnderlyingObject(V, TD) : V->stripPointerCasts();
   if (LoadInst *L = dyn_cast<LoadInst>(V)) {
     BasicBlock::iterator BBI = L;

The new version of isNoopCast should be usable here.

===================================================================
--- lib/Analysis/ConstantFolding.cpp    (revision 166296)
+++ lib/Analysis/ConstantFolding.cpp    (working copy)
@@ -377,11 +377,12 @@
   }

   if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
-    if (CE->getOpcode() == Instruction::IntToPtr &&
-        CE->getOperand(0)->getType() == TD.getIntPtrType(CE->getContext()))
+    if (CE->getOpcode() == Instruction::IntToPtr) {
+      if (CE->getOperand(0)->getType() == TD.getIntPtrType(CE->getType()))
       return ReadDataFromGlobal(CE->getOperand(0), ByteOffset, CurPtr,
                                 BytesLeft, TD);
   }
+  }

   // Otherwise, unknown initializer type.
   return false;

Why split the if statement?

@@ -988,9 +992,12 @@
   // ConstantExpr::getCompare cannot do this, because it doesn't have TD
   // around to know if bit truncation is happening.
   if (ConstantExpr *CE0 = dyn_cast<ConstantExpr>(Ops0)) {
+    Type *IntPtrTy = NULL;
+    unsigned AS = 0;
     if (TD && Ops1->isNullValue()) {
-      Type *IntPtrTy = TD->getIntPtrType(CE0->getContext());
       if (CE0->getOpcode() == Instruction::IntToPtr) {
+        AS = CE0->getType()->getPointerAddressSpace();
+        IntPtrTy = TD->getIntPtrType(CE0->getContext(), AS);
         // Convert the integer value to the right size to ensure we get the
         // proper extension or truncation.
         Constant *C = ConstantExpr::getIntegerCast(CE0->getOperand(0),

You can simplify this and the following code with the new
getIntPtrType overload.

-Eli

-Eli

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 002-per-address-space-get-int-ptr-type-patch.txt
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121019/5107d7ce/attachment.txt>