[PATCH] Address Space Casting
Matt Arsenault
Matthew.Arsenault at amd.com
Mon Aug 19 12:52:33 PDT 2013
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
More information about the llvm-commits
mailing list