[llvm-commits] [PATCH] update getPointerTo to handle multiple address spaces

[Villmow, Micah]

I would prefer to wait on doing any major refactoring. This is way outside of the original scope of the work that I had original planned/agreed on doing. This original scope of the work was basically:
1) Move TargetData to DataLayout.
2) Update all clients.
3) Add in support for multiple address spaces to DataLayout.
4) Update clients to add in support for multiple address spaces having varying pointer sizes.

Now, ChrisL wants DataLayout removed from being a pass and moved into the Module(Bug11936), and now you want a refactor of the pointer types. Now, after having dealing with this for the last few months, I agree, the code is not as clean as it could be, but stopping everything just to re-factor, I don't agree with, when most of the changes have already gone in.

Otherwise I'll just be stopping one set of work when it is almost done to move on to another project that again is not guaranteed to not be affected by the same scope creep. LLVM 3.2 branches in about 2 weeks, and as it currently is, multiple address space support is not complete, but almost finished. I would highly prefer to get it working by 3.2 and then refactor the code that has shown to be problematic.

More comments inline.

Micah
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Chandler Carruth
> Sent: Monday, October 29, 2012 1:16 PM
> To: reviews+D85+public+32bd68782163e28d at llvm-reviews.chandlerc.com
> Cc: Commit Messages and Patches for LLVM; Micah Villmow
> Subject: Re: [llvm-commits] [PATCH] update getPointerTo to handle
> multiple address spaces
> 
> I completely agree with Eli's comments here.
> 
> The more of these patches I see, the more the API for address spaces
> looks *wrong*. I want to go and fix these APIs first, and I am opposed
> to *any* further churn to switch client code until we finish fixing
> them.
> 
> Some concrete next steps:
> 
> 1) The type used to "base" a pointer type on must *always* be a
> pointer. Please go back and implement my (and Duncan, and others')
> suggestion to assert if the type is anything other than a pointer or
> vector of pointers. This bit of code review has been neglected for
> over a week as churn and chaos develop in the codebase. That is
> unacceptable.
[Villmow, Micah] This was not true about the code base before I started work, so why would it be different now?
Ty->getPointerTo() was implemented as return llvm::PointerType::get(this, 0);, with no requirement that 'this' is a pointer.  This implementation doesn't change, I only remove the default argument and make it explicit. For the new interface, I only added it as a shortcut method that helped clean up the code base because of a similar change to getIntPtrTy as requested by Eli. This reduces the amount of ternary operators in the code quite dramatically. By making this code ONLY accept a pointer type, then there is really no point in having the helper function, as the ternary operators would then be introduced in the caller.
> 
> 2) You should look at integrating the address space and pointer type
> creation with IRBuilder the same way that integer type creation is
> built into it. We don't have to pass the LLVM Context object around
> everywhere as a consequence, and I suspect that a similar simplifying
> pattern will emerge where we can set the default address space in the
> IRBuilder (or other builder class) and re-use it pervasively rather
> than having 0U encoded everywhere (which is still better than random
> recursive type references!)
[Villmow, Micah] As I mentioned above, I agree that this code needs cleanup, but this is outside of the scope of work related to variable pointer sizes.
> 
> On Fri, Oct 26, 2012 at 3:24 PM, Micah Villmow <villmow at gmail.com>
> wrote:
> > This patch updates getPointerTo to handle multiple address spaces.
> The original request was here:
> > http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-
> 20121022/154243.html
> >
> > http://llvm-reviews.chandlerc.com/D85
> >
> > Files:
> >   lib/Transforms/Vectorize/LoopVectorize.cpp
> >   lib/Transforms/Instrumentation/ThreadSanitizer.cpp
> >   lib/Transforms/Instrumentation/GCOVProfiling.cpp
> >   lib/Transforms/Instrumentation/ProfilingUtils.cpp
> >   lib/Transforms/Scalar/SROA.cpp
> >   lib/VMCore/Constants.cpp
> >   lib/VMCore/Type.cpp
> >   lib/VMCore/Verifier.cpp
> >   lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
> >   lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
> >   lib/ExecutionEngine/ExecutionEngine.cpp
> >   include/llvm/Type.h
> >   examples/ExceptionDemo/ExceptionDemo.cpp
> >   tools/clang/lib/CodeGen/ItaniumCXXABI.cpp
> >   tools/clang/lib/CodeGen/CGObjCMac.cpp
> >   tools/clang/lib/CodeGen/CGExprScalar.cpp
> >   tools/clang/lib/CodeGen/CGExpr.cpp
> >   tools/clang/lib/CodeGen/CGVTables.cpp
> >   tools/clang/lib/CodeGen/CGClass.cpp
> >   tools/clang/lib/CodeGen/CGBuiltin.cpp
> >   tools/clang/lib/CodeGen/CGDecl.cpp
> >   tools/clang/lib/CodeGen/CGCXXABI.cpp
> >   tools/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
> >   tools/clang/lib/CodeGen/CGException.cpp
> >   tools/clang/lib/CodeGen/CGCXX.cpp
> >   tools/clang/lib/CodeGen/CGObjC.cpp
> >   tools/clang/lib/CodeGen/CodeGenTypes.cpp
> >   tools/clang/lib/CodeGen/CodeGenModule.cpp
> >   tools/clang/lib/CodeGen/CGExprCXX.cpp
> >   tools/clang/lib/CodeGen/CGCall.cpp
> >   tools/clang/lib/CodeGen/CGBlocks.cpp
> >   tools/clang/lib/CodeGen/CGObjCRuntime.cpp
> >   tools/dragonegg/src/Backend.cpp
> >   tools/dragonegg/src/TypeConversion.cpp
> >   tools/dragonegg/src/ConstantConversion.cpp
> >   tools/dragonegg/src/x86/Target.cpp
> >   tools/dragonegg/src/Convert.cpp
> >   tools/dragonegg/src/DefaultABI.cpp
> >   tools/lldb/source/Expression/IRForTarget.cpp
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits