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

[Villmow, Micah]

Eli,
 I've made the requested modifications and this patch looks a lot cleaner with the new API changes.

Micah
________________________________________
From: Eli Friedman [eli.friedman at gmail.com]
Sent: Thursday, October 18, 2012 8:31 PM
To: Villmow, Micah
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] Patch to add address space support to getIntPtrTy

On Thu, Oct 18, 2012 at 4:10 PM, Villmow, Micah <Micah.Villmow at amd.com> wrote:
> Weird, ok, attached the re-updated new one.

Since you mentioned it, I think getIntPtrTy(Type*) is a very good
idea.  It would simplify a bunch of changes in this patch.  (I would
prefer Type* over Value* because the meaning is more obvious.)

Index: lib/Analysis/ScalarEvolution.cpp
===================================================================
--- lib/Analysis/ScalarEvolution.cpp    (revision 166195)
+++ lib/Analysis/ScalarEvolution.cpp    (working copy)
@@ -2586,7 +2586,7 @@
   // constant expression and then folding it back into a ConstantInt.
   // This is just a compile-time optimization.
   if (TD)
-    return getConstant(TD->getIntPtrType(getContext()),
+    return getConstant(TD->getIntPtrType(getContext(), 0),
                        TD->getTypeAllocSize(AllocTy));

   Constant *C = ConstantExpr::getSizeOf(AllocTy);

I think this needs to take the integer type from the caller.

@@ -2612,7 +2612,7 @@
   // constant expression and then folding it back into a ConstantInt.
   // This is just a compile-time optimization.
   if (TD)
-    return getConstant(TD->getIntPtrType(getContext()),
+    return getConstant(TD->getIntPtrType(getContext(), 0),
                        TD->getStructLayout(STy)->getElementOffset(FieldNo));

   Constant *C = ConstantExpr::getOffsetOf(STy, FieldNo);

Same here.

Index: lib/CodeGen/SelectionDAG/FastISel.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/FastISel.cpp       (revision 166195)
+++ lib/CodeGen/SelectionDAG/FastISel.cpp       (working copy)
@@ -100,10 +100,16 @@
     return false;

   // 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)) {
+    unsigned AS = 0;
+    if (Cast->getOpcode() == Instruction::PtrToInt)
+      AS = Cast->getOperand(0)->getType()->getPointerAddressSpace();
+    else if (Cast->getOpcode() == Instruction::IntToPtr)
+      AS = Cast->getType()->getPointerAddressSpace();
+    if (Cast->isNoopCast(TD.getIntPtrType(Cast->getContext(), AS)) &&
         !hasTrivialKill(Cast->getOperand(0)))
       return false;
+    }

Can you change isNoopCast to take a DataLayout or something like that?
 This just makes the code unreadable.

Index: lib/CodeGen/SelectionDAG/SelectionDAG.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAG.cpp   (revision 166195)
+++ lib/CodeGen/SelectionDAG/SelectionDAG.cpp   (working copy)
@@ -3804,7 +3804,8 @@
   // Emit a library call.
   TargetLowering::ArgListTy Args;
   TargetLowering::ArgListEntry Entry;
-  Entry.Ty = TLI.getDataLayout()->getIntPtrType(*getContext());
+  unsigned AS = SrcPtrInfo.getAddrSpace();
+  Entry.Ty = TLI.getDataLayout()->getIntPtrType(*getContext(), AS);
   Entry.Node = Dst; Args.push_back(Entry);
   Entry.Node = Src; Args.push_back(Entry);
   Entry.Node = Size; Args.push_back(Entry);

This pattern seems common enough that you should just add a
getIntPtrType somewhere that takes a MachinePointerInfo.

Index: lib/Transforms/InstCombine/InstCombineCalls.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineCalls.cpp     (revision 166195)
+++ lib/Transforms/InstCombine/InstCombineCalls.cpp     (working copy)
@@ -996,9 +996,13 @@
         // Conversion is ok if changing from one pointer type to
another or from
         // a pointer to an integer of the same size.
         !((OldRetTy->isPointerTy() || !TD ||
-           OldRetTy == TD->getIntPtrType(Caller->getContext())) &&
+            // FIXME: Not sure what to do here, so setting AS to 0.
+            // How can the AS for a function call be outside the default?
+           OldRetTy == TD->getIntPtrType(Caller->getContext(), 0)) &&
           (NewRetTy->isPointerTy() || !TD ||
-           NewRetTy == TD->getIntPtrType(Caller->getContext()))))
+            // FIXME: Not sure what to do here, so setting AS to 0.
+            // How can the AS for a function call be outside the default?
+           NewRetTy == TD->getIntPtrType(Caller->getContext(), 0))))
       return false;   // Cannot transform this return value.

     if (!Caller->use_empty() &&

This code is trying to compare OldRetTy and NewRetTy.  So the relevant
check is something like "OldRetTy == TD->getIntPtrType(NewRetTy)",
assuming the new getIntPtrTy(Type*) utility.

Index: lib/Transforms/InstCombine/InstructionCombining.cpp
===================================================================
--- lib/Transforms/InstCombine/InstructionCombining.cpp (revision 166195)
+++ lib/Transforms/InstCombine/InstructionCombining.cpp (working copy)
@@ -746,7 +746,8 @@
   // Start with the index over the outer type.  Note that the type size
   // might be zero (even if the offset isn't zero) if the indexed type
   // is something like [0 x {int, int}]
-  Type *IntPtrTy = TD->getIntPtrType(Ty->getContext());
+  // FIXME: Not sure what to do here, so setting address space to default.
+  Type *IntPtrTy = TD->getIntPtrType(Ty->getContext(), 0);
   int64_t FirstIdx = 0;
   if (int64_t TySize = TD->getTypeAllocSize(Ty)) {
     FirstIdx = Offset/TySize;

The integer type needs to come from the caller.

-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/c572b165/attachment.txt>