[PATCH] Teach SimplifyCFG about address spaces

Matt Arsenault Matthew.Arsenault at amd.com
Wed Oct 16 11:43:29 PDT 2013



================
Comment at: test/Transforms/SimplifyCFG/switch_create.ll:4
@@ -2,1 +3,3 @@
+
+; TODO: Other tests should also have check lines with datalayout
 
----------------
Quentin Colombet wrote:
> When do you plan to do that?
I've been waiting for my allow multiple prefix FileCheck patch to be committed to fix things like this.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:481
@@ +480,3 @@
+      Value *Ptr = PTII->getPointerOperand();
+      if (PTII->getType() == TD->getIntPtrType(Ptr->getType()))
+        CV = Ptr;
----------------
Quentin Colombet wrote:
> Although this is correct (and equivalent to what I am about to propose), I wonder if it would not be better (when reading the code) to check for a context + address space instead of using the type interface.
> 
> Indeed, this would make explicit what we are checking here.
> 
> That said, accessing the pointerTy to get the address space will create more code.
> 
> My point is I’m fine with this code, I was just wondering if we can make it more readable based on the API documentation. For instance, I had to dig into the implementation of DataLayout to check that this call was doing the same thing as the context + address space call.
> Another option may to mention the address space in the documentation of DataLayout::getIntPtrType(Type*).
I don't consistently prefer either way. I'll usually use the address space if I already have a variable around with it. Usually the reviews I get say the opposite though.

The only time I think the type way is better is when there could be a vector of pointers, and the type way will do the correct thing. The address space version returns the scalar pointer type.


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



More information about the llvm-commits mailing list