[PATCH] D31042: Allow DataLayout to specify addrspace for allocas.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:1098
+  AllocaInst *CreateAlloca(const DataLayout &DL, Type *Ty,
+                           Value *ArraySize = nullptr, const Twine &Name = "") {
+    return Insert(new AllocaInst(Ty, DL.getStackAddrSpace(), ArraySize), Name);
----------------
arsenm wrote:
> efriedma wrote:
> > I'm not sure this overload is a good idea; it saves a little typing, but it hides the reason we need the datalayout.  
> I'm not sure that's a problem here. This is the most common version, and the point of IRBuilder is to be the convenient way of creating IR. A DataLayout argument is less error prone than an unsigned argument, where it easier to accidentally pass the alignment or something else.
> 
> It would also be able to hide this more by getting the parent module's DataLayout from the insertion block (which is what the CHERI patches did). However there are already functions using DataLayout input arguments, so I figured there might be some reason to pass it in rather than doing that.
I guess.  The part that concerns me is that the type of the result might not be what someone expects if they aren't paying attention, and the resulting assertion failure is a bit unfriendly... but I guess we can go with this for now, and see if it ends up being confusing in practice.

Getting the DataLayout from the insertion block doesn't work for all methods on IRBuilder; an IRBuilder isn't guaranteed to always have an insertion point.  This can be used, for example, if you're building a constant.  I guess for alloca in particular, it would be safe to assume we have an insertion point.


https://reviews.llvm.org/D31042