[RFC] Remove User::OperandList

[Pete Cooper]

> On May 29, 2015, at 10:31 AM, Owen Anderson <resistor at mac.com> wrote:
> 
> A few more comments:
> 
> 
>> 
>> PHINode::~PHINode() {
>> -  dropHungoffUses();
>> }
>> 
>> // removeIncomingValue - Remove an incoming value.  This is useful if a
>> @@ -191,7 +190,6 @@ LandingPadInst::LandingPadInst(const LandingPadInst &LP)
>> }
>> 
>> LandingPadInst::~LandingPadInst() {
>> -  dropHungoffUses();
>> }
>> 
>> LandingPadInst *LandingPadInst::Create(Type *RetTy, Value *PersonalityFn,
>> @@ -3325,7 +3323,6 @@ SwitchInst::SwitchInst(const SwitchInst &SI)
>> }
>> 
>> SwitchInst::~SwitchInst() {
>> -  dropHungoffUses();
>> }
>> 
>> 
>> @@ -3450,7 +3447,6 @@ IndirectBrInst::IndirectBrInst(const IndirectBrInst &IBI)
>> }
>> 
>> IndirectBrInst::~IndirectBrInst() {
>> -  dropHungoffUses();
>> }
> 
> 
> Can these just be deleted?
Yep, i’ll add another patch to the list to do that, or merge it in to the patch which removed those calls.
> 
>> +void User::reallocHungoffUses(unsigned NewNumUses, bool IsPhi) {
>> +  assert(HasHungOffUses && "realloc must have hung off uses");
>> +
>> +  unsigned OldNumUses = getNumOperands();
>> +
>> +  // We don't support shrinking the number of uses.  We wouldn't have enough
>> +  // space to copy the old uses in to the new space.
>> +  assert(NewNumUses > OldNumUses && "realloc must grow num uses");
>> +
>> +  Use *OldOps = OperandList;
>> +  allocHungoffUses(NewNumUses, IsPhi);
> 
> It would require changing the allocation scheme to malloc/free, but it seems a bit silly that we have a method called realloc… that doesn’t use realloc.
Good point :)  Since I require that the new use count exceeds the existing one, I could just rename it to growHungOffUses.
> 
>> Finally, fixed all callers of the User::new to call the correct version of the method.  This involved adding (unsigned) casts or changing for example 1 to 1U in callers to disambiguate the 'new' we want.
> 
> This seems kind of gross.  Is there any cleaner way to get the same effect?
Yeah, I agree.  I considered just adding the bool to the end of the existing User::new and making it default to false.  Then I could assert in there that we ask for 0 Use’s if we need hung off uses.  That would mean I only need to rewrite the hung off use calls to new.

Cheers,
Pete
> 
> 
> —Owen
> 
> 
>> On May 28, 2015, at 11:30 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>> 
>> Hi all
>> 
>> This series of patches removes around 2.5% of peak memory by computing OperandList instead of always storing it in User.
>> 
>> The majority of the patches are cleanup and refactoring to make OperandList and NumOperands be managed entirely in User and not have subclasses changing their values directly.  The first few patches also move all the ‘hung off uses’ logic for PhiNode/SwitchInst/LandingPadInst/IndirectBr in to User as that code was particularly heavy on hacking on the OperandList.
>> 
>> The OperandList itself was never actually required for all subclasses of User.  The hung off cases needed it as they actually allocate memory for operands, but the majority of instructions for example allocate a fixed size array of Use intrusively in User::new.  We can get to the first operand for these instructions by subtracting (NumOperands * sizeof(Use)) from the User this pointer.  We use the pointer arithmetic on these cases, and in the hung off case we allocate the Use* in User::new instead of the Use[N] array.  This requires that we have another User::new for hung off uses.
>> 
>> The main change here is actually in 0008 which changes getOperandList() from
>> 
>> Use *getOperandList() const {
>> return LegacyOperandList;
>> }
>> 
>> to
>> 
>> const Use *getOperandList() const {
>>   if (HasHungOffUses) {
>>     Use * const*Storage = reinterpret_cast<Use* const*>(this) - 1;
>>     return *Storage;
>>   }
>>   const Use *Storage = reinterpret_cast<const Use*>(this) - NumOperands;
>>   return Storage;
>> }
>> 
>> The numbers are also in 0008, but to give them here:
>> 
>> On 'opt -O2 verify-uselistorder.lto.bc', peak memory usage prior to this change is 149MB and after is 143MB so the savings are around 2.5% of peak.
>> 
>> Looking at some passes which allocate many Instructions and Values, parseIR drops from 54.25MB to 52.21MB while the Inliner calls to Instruction::clone() drops from 28.20MB to 27.05MB.
>> 
>> Comments welcome.
>> 
>> Cheers,
>> Pete
>> 
>> <0001-Move-the-special-Phi-logic-for-hung-off-uses-in-to-U.patch><0002-Make-User-track-whether-a-class-has-hung-off-uses-an.patch><0003-Delete-User-dropHungOffUses-and-move-it-in-to-User-w.patch><0004-Add-User-reallocHungoffUses-and-use-it-to-grow-the-h.patch><0005-Stop-returning-a-Use-from-allocHungOffUses.patch><0006-Added-a-version-of-User-new-for-hung-off-uses.patch><0007-Replace-all-accesses-to-User-OperandList-with-getter.patch><0008-Move-OperandList-to-be-allocated-prior-to-User-for-h.patch><0009-Rename-NumOperands-to-make-it-clear-its-managed-by-t.patch>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>