[PATCH] Address Space Casting

Michele Scandale michele.scandale at gmail.com
Mon Aug 19 13:33:34 PDT 2013



================
Comment at: lib/IR/Instructions.cpp:2299
@@ +2298,3 @@
+             "Illegal addrspacecast, bitcast sequence!"); 
+      // allowed, use first cast's opcode
+      return firstOp;
----------------
Matt Arsenault wrote:
> Comments should use proper capitalization and punctuation
Is the same as the comments in other cases (see case 1 and case 2)... Should I fix all the cases?

================
Comment at: lib/IR/Instructions.cpp:2308-2309
@@ +2307,4 @@
+             DstTy->isPointerTy() && 
+           SrcTy->getPointerAddressSpace() == MidTy->getPointerAddressSpace() &&
+           MidTy->getPointerAddressSpace() != DstTy->getPointerAddressSpace() &&
+             "Illegal bitcast, addrspacecast sequence!"); 
----------------
Matt Arsenault wrote:
> These look weirdly intended relative to the parts before it
Was a not so good choice to not break the 80 columns limit.

Is it better something like this?

  assert(
    SrcTy->isPointerTy() &&
    MidTy->isPointerTy() &&
    DstTy->isPointerTy() &&
    SrcTy->getPointerAddressSpace() == MidTy->getPointerAddressSpace() &&
    MidTy->getPointerAddressSpace() != DstTy->getPointerAddressSpace() &&
    "Illegal bitcast, addrspacecast sequence!");

================
Comment at: lib/Target/X86/X86ISelLowering.h:766
@@ -765,1 +765,3 @@
 
+    virtual bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const;
+
----------------
Matt Arsenault wrote:
> LLVM_OVERRIDE
This is not used on other similar cases... I just kept the local style.

================
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
----------------
Matt Arsenault wrote:
> This one isn't aligned with the rest, but the others are
Also the other CastInsts have this misalignement. I think that a separate patch should fix these code-style issues.


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



More information about the llvm-commits mailing list