[PATCH] D31058: Store Arguments in a flat array instead of an iplist

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: include/llvm/IR/Argument.h:33
   Function *Parent;
   unsigned ArgNo;
 
----------------
sanjoy wrote:
> Do we need to store both?  Why not have `getArgNo` return `this - Parent->getArgBegin()` (or equivalent)?
Sounds like a reasonable follow-up. I just added ArgNo in a previous CL, which was the minimal thing necessary to fix the non-linear behavior.


================
Comment at: include/llvm/IR/Function.h:51
 public:
-  typedef SymbolTableList<Argument> ArgumentListType;
+  typedef OwningArrayRef<Argument> ArgumentListType;
   typedef SymbolTableList<BasicBlock> BasicBlockListType;
----------------
chandlerc wrote:
> You're not really using this in the way I would expect -- instead we use a raw pointer for the storage. We just return a const reference to this. That doesn't make a lot of sense.
> 
> I think you want to change the getArgumentList methods to operate in terms of ArrayRef, and either use this for the storage itself or not have it at all.
Oh, this is stale. Oops. I wasn't able to use OwningArrayRef because it tries to default construct its contents, and Argument doesn't have a default ctor, so... it seemed better to use malloc and do the low-level thing.


================
Comment at: include/llvm/IR/Function.h:107-110
+  /// Internal wrapper for conveniently iterating the argument list.
+  MutableArrayRef<Argument> getArgList() {
+    return MutableArrayRef<Argument>(Arguments, NumArgs);
+  }
----------------
chandlerc wrote:
> chandlerc wrote:
> > We already have getArgumentList?
> I see you're removing in another patch... Maybe rewrite the users of this in terms of arg_begin too?
I can't, arg_begin() triggers lazy argument list creation. This is a helper intended for use inside stealArgumentListFrom, which doesn't want lazy argument list creation. This is a private method, fwiw. I could sink it to be a static helper that takes Arguments and NumArgs? We just don't have makeMutableArrayRef(...).


================
Comment at: lib/IR/Function.cpp:230-231
+  if (NumArgs > 0) {
+    Arguments =
+        reinterpret_cast<Argument *>(malloc(sizeof(Argument) * NumArgs));
+    for (unsigned i = 0, e = NumArgs; i != e; ++i) {
----------------
chandlerc wrote:
> Why malloc?
Is there a way to use 'new' to allocate uninitialized memory suitable for this kind of placement new construction? I can do `new char[sizeof(Argument)*NumArgs]`, but it didn't seem prettier. Regular array new wants a default ctor, which Argument doesn't have, and adding one would be wasteful. I just want to allocate some memory and initialize some complex, non-value type objects into it. How hard can that be? ¯\_(ツ)_/¯


https://reviews.llvm.org/D31058