[PATCH] Address Space Casting

Matt Arsenault Matthew.Arsenault at amd.com
Mon Aug 19 15:38:59 PDT 2013



================
Comment at: lib/Target/X86/X86ISelLowering.h:766
@@ -765,1 +765,3 @@
 
+    virtual bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const;
+
----------------
Michele Scandale wrote:
> Matt Arsenault wrote:
> > LLVM_OVERRIDE
> This is not used on other similar cases... I just kept the local style.
It's not really a style thing. It prevents accidentally doing the wrong thing, like if you typoed it as virtual isNopoAddrSpaceCast, it wouldn't actually do what you expect and this catches that. Not enough places use it yet.

Also do you have a test for this x86 part?


================
Comment at: lib/IR/Instructions.cpp:2308-2309
@@ +2307,4 @@
+             DstTy->isPointerTy() && 
+           SrcTy->getPointerAddressSpace() == MidTy->getPointerAddressSpace() &&
+           MidTy->getPointerAddressSpace() != DstTy->getPointerAddressSpace() &&
+             "Illegal bitcast, addrspacecast sequence!"); 
----------------
Michele Scandale wrote:
> 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!");
Sure

================
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
----------------
Michele Scandale wrote:
> 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.
Sure, but I don't think you should introduce new ones in places you're touching

================
Comment at: lib/IR/Instructions.cpp:2299
@@ +2298,3 @@
+             "Illegal addrspacecast, bitcast sequence!"); 
+      // allowed, use first cast's opcode
+      return firstOp;
----------------
Michele Scandale wrote:
> 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?
Yes. There are also a few others here


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



More information about the llvm-commits mailing list