This is an archive of the discontinued LLVM Phabricator instance.

Store Arguments in a flat array instead of an iplist
ClosedPublic

Authored by rnk on Mar 16 2017, 2:57 PM.

Details

Summary

This saves two pointers from Argument and eliminates some extra
allocations.

Arguments cannot be inserted or removed from a Function because that
would require changing its Type, which LLVM does not allow. Instead,
passes that change prototypes, like DeadArgElim, create a new Function
and copy over argument names and attributes. The primary benefit of
iplist is O(1) random insertion and removal. We just don't need that for
arguments, so don't use it.

Event Timeline

rnk created this revision.Mar 16 2017, 2:57 PM
chandlerc added inline comments.Mar 16 2017, 3:29 PM
include/llvm/IR/Function.h
50–51

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.

106–109

We already have getArgumentList?

lib/IR/Function.cpp
230–231

Why malloc?

sanjoy added a subscriber: sanjoy.Mar 16 2017, 3:38 PM
sanjoy added inline comments.
include/llvm/IR/Argument.h
33

Do we need to store both? Why not have getArgNo return this - Parent->getArgBegin() (or equivalent)?

chandlerc added inline comments.Mar 16 2017, 3:45 PM
include/llvm/IR/Function.h
106–109

I see you're removing in another patch... Maybe rewrite the users of this in terms of arg_begin too?

rnk added inline comments.Mar 16 2017, 4:23 PM
include/llvm/IR/Argument.h
33

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.

include/llvm/IR/Function.h
50–51

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.

106–109

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(...).

lib/IR/Function.cpp
230–231

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? ¯\_(ツ)_/¯

rnk updated this revision to Diff 92085.Mar 16 2017, 4:39 PM
  • Delete stale typedef, sink arrayref helper
rnk added inline comments.Mar 16 2017, 4:51 PM
lib/IR/Function.cpp
230–231

Maybe you're suggesting ::operator new(...)? That'd work.

chandlerc accepted this revision.Mar 16 2017, 5:54 PM

LGTM with whatever you decide about the allocator stuff.

lib/IR/Function.cpp
230–231

Is std::allocator<Argument>().allocate(NumArgs) too verbose/arcane? It has the semantics I think we want here. (And `.deallocate(...)' below.)

Anyways, I don't feel particularly strongly about this.

245–247

We should either have makeArrayRef produce a MutableArrayRef for non-const pointers or we should add a makeMutableArrayRef that accepts non-const pointers. Then I don't think you'll need this.

That can bea follow-up cleanup.

This revision is now accepted and ready to land.Mar 16 2017, 5:54 PM
rnk added inline comments.Mar 17 2017, 10:28 AM
lib/IR/Function.cpp
230–231

Nope, sounds good, gets rid of the reinterpret_cast. Thanks!

245–247

I think we want the second, or we'll need to add a conversion from MutableArrayRef<T> to ArrayRef<T> to avoid breaking callers of makeArrayRef with non-const pointers. MutableArrayRef is pretty niche, though, so I'm gonna leave it alone for now.

This revision was automatically updated to reflect the committed changes.