[PATCH] Address Space Casting

[Matt Arsenault]

  Some nitpicks


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp:491-495
@@ -488,1 +490,7 @@
+               dyn_cast<AddrSpaceCastSDNode>(this)) {
+    OS << "[" 
+       << ASC->getSrcAddressSpace() 
+       << " -> " 
+       << ASC->getDestAddressSpace()
+       << "]";
   }
----------------
You should use single quotes for single characters

================
Comment at: lib/IR/Constants.cpp:1492-1493
@@ -1489,2 +1491,4 @@
 
+  unsigned SrcAS = S->getType()->getPointerAddressSpace();
+
   if (Ty->isIntOrIntVectorTy())
----------------
Move this down where it is actually used

================
Comment at: lib/IR/Constants.cpp:1494-1497
@@ -1490,3 +1493,6 @@
+
   if (Ty->isIntOrIntVectorTy())
     return getPtrToInt(S, Ty);
+  else if (Ty->isPointerTy() && SrcAS != Ty->getPointerAddressSpace())
+    return getAddrSpaceCast(S, Ty);
   return getBitCast(S, Ty);
----------------
Return before else

================
Comment at: lib/IR/AutoUpgrade.cpp:395-396
@@ +394,4 @@
+unsigned llvm::UpgradeCastOpcode(unsigned Opc, Value *V, Type *DestTy) {
+  Type *SrcTy = V->getType();
+  if (Opc == Instruction::BitCast) {
+    if (SrcTy->isPointerTy() && DestTy->isPointerTy() &&
----------------
This can be early return

================
Comment at: include/llvm/IR/Instructions.h:3755
@@ +3754,3 @@
+    Value *S,                     ///< The value to be casted
+    Type *Ty,               ///< The type to casted to
+    const Twine &NameStr = "",    ///< A name for the new instruction
----------------
This one isn't aligned with the rest, but the others are

================
Comment at: include/llvm/IR/Instructions.h:3763
@@ +3762,3 @@
+    Value *S,                     ///< The value to be casted
+    Type *Ty,               ///< The type to casted to
+    const Twine &NameStr,         ///< A name for the new instruction
----------------
Same thing

================
Comment at: lib/IR/Instructions.cpp:2241
@@ -2236,1 +2240,3 @@
+      // FIXME: Is this always true?
+      if (MidSize == 64) return Instruction::BitCast;
 
----------------
Return on same line

================
Comment at: lib/IR/Instructions.cpp:2240
@@ +2239,3 @@
+      // pointer size.
+      // FIXME: Is this always true?
+      if (MidSize == 64) return Instruction::BitCast;
----------------
There are a bunch of places scattered around that assume this, they probably should all be fixed at once. I was thinking of adding a getMaxPointerSize or something.

================
Comment at: lib/IR/Instructions.cpp:2447
@@ -2425,1 +2446,3 @@
 
+  Type *STy = S->getType();
+
----------------
Move this down to where it is used

================
Comment at: lib/IR/Instructions.cpp:2449-2451
@@ -2426,2 +2448,5 @@
+
   if (Ty->isIntOrIntVectorTy())
     return Create(Instruction::PtrToInt, S, Ty, Name, InsertAtEnd);
+  else if (STy->getPointerAddressSpace() != Ty->getPointerAddressSpace())
+    return Create(Instruction::AddrSpaceCast, S, Ty, Name, InsertAtEnd);
----------------
Return before else

================
Comment at: lib/IR/Instructions.cpp:2468-2473
@@ -2442,4 +2467,8 @@
 
+  Type *STy = S->getType();
+
   if (Ty->isIntOrIntVectorTy())
     return Create(Instruction::PtrToInt, S, Ty, Name, InsertBefore);
+  else if (STy->getPointerAddressSpace() != Ty->getPointerAddressSpace())
+    return Create(Instruction::AddrSpaceCast, S, Ty, Name, InsertBefore);
   return Create(Instruction::BitCast, S, Ty, Name, InsertBefore);
----------------
Same things

================
Comment at: lib/Target/X86/X86ISelLowering.h:766
@@ -765,1 +765,3 @@
 
+    virtual bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const;
+
----------------
LLVM_OVERRIDE

================
Comment at: lib/IR/Instructions.cpp:2299
@@ +2298,3 @@
+             "Illegal addrspacecast, bitcast sequence!"); 
+      // allowed, use first cast's opcode
+      return firstOp;
----------------
Comments should use proper capitalization and punctuation

================
Comment at: lib/IR/Instructions.cpp:2308-2309
@@ +2307,4 @@
+             DstTy->isPointerTy() && 
+           SrcTy->getPointerAddressSpace() == MidTy->getPointerAddressSpace() &&
+           MidTy->getPointerAddressSpace() != DstTy->getPointerAddressSpace() &&
+             "Illegal bitcast, addrspacecast sequence!"); 
----------------
These look weirdly intended relative to the parts before it

================
Comment at: lib/IR/Verifier.cpp:936
@@ -938,1 +935,3 @@
+          "Bitcasts between pointers of different address spaces is not legal."
+          "Use instead AddrSpaceCast instruction.", V);
 }
----------------
These words sound backwards. "Use AddrSpaceCast instead"


http://llvm-reviews.chandlerc.com/D1401