[RFC] Remove User::OperandList

[Duncan P. N. Exon Smith]

> On 2015 Jun 9, at 18:12, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> Attached new versions of the patches based on both of your feedback.
> 

Be nice to run clang-format on some of these prior to commit.  It would
canonicalize some whitespace while you're editing the lines anyway (I
see a few minor things on the lines you've edited, mostly not things
that you added though FWIW).

> 0001 - unchanged

> +    return User::allocHungoffUses(N, true);

Please add a comment like:

    return User::allocHungoffUses(N, /* IsPhi */ true);

LGTM with that.

> 0002 - steal a bit from NumOperands instead of SubclassOptionalData

Is there an assertion anywhere that the number of operands actually
fits inside `NumOperands`?  If so, update it; otherwise, add one.

Otherwise LGTM.

> 0003 - Delete the destructors too as ~PHINode for example was now empty

I might move `User::~User()` to the source file.  LGTM either way.

> 0004 - Renamed reallocHungOffUses to growHungoffUses

> +  growHungoffUses(ReservedSpace, true);

Please add a comment like:

    growHungoffUses(ReservedSpace, /* IsPhi */ true);

LGTM with that.

> 0005 - unchanged

LGTM.

> 0006 - Add User::new(size_t) instead of an override taking a bool.

> +void *User::operator new(size_t s) {
> +  const unsigned Us = 0;
> +  void *Storage = ::operator new(s + sizeof(Use) * Us);
> +  Use *Start = static_cast<Use*>(Storage);

clang-format would fix up the whitespace.

> +  Use *End = Start + Us;
> +  User *Obj = reinterpret_cast<User*>(End);
> +  Obj->OperandList = Start;
> +  Obj->HasHungOffUses = true;
> +  Obj->NumOperands = Us;
> +  Use::initTags(Start, End);
> +  return Obj;

This logic looks all wrong to me.  Firstly, `Us` is always 0, so you can
factor it out.

More importantly, why are you co-allocating a `Use`, when the uses are
hung-off?  Shouldn't this co-allocate a `Use*`, if anything?  Or does
that wait for 0008?

It also seems strange/wrong to be pointing `OperandList` at anything
here -- I feel like it should start out as `nullptr`.

Maybe this would make more sense squashed with 0008.  Let me know if I'm
just missing something, too.

> +}
> +
> 

> 0007 - unchanged

Except for the changes that depend on 0006 (and I think these can just
be reordered?), this LGTM.

> 0008 - unchanged
> 0009 - unchanged

I didn't look at these; I'll wait to see what you do with 0006.

> 
> Comments appreciated.  
> 
> Also, if any of the earlier patches are ok and can be committed then that would be great so i don’t rebase too much.  0008 should be the only real difference.  Everything else is mostly refactoring.
> 
> Thanks,
> 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-growHungoffUses-and-use-it-to-grow-the-hung.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>
> 
>> On Jun 1, 2015, at 9:53 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>> 
>> 
>>> On Jun 1, 2015, at 9:50 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>> 
>>>> On 2015-May-29, at 10:35, Pete Cooper <peter_cooper at apple.com> wrote:
>>>> 
>>>> 
>>>>> On May 29, 2015, at 10:31 AM, Owen Anderson <resistor at mac.com> wrote:
>>>>> 
>>>>>> 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.
>>> 
>>> This rejected alternative sounds better to me.
>>> 
>>> Alternatively, you could avoid the `bool` altogether:
>>> 
>>>  /// Allocate a User with an operand pointer co-allocated.
>>>  void *operator new(size_t Size);
>>> 
>>>  /// Allocate a User with the operands co-allocated.
>>>  void *operator new(size_t Size, unsigned NumOps);
>>> 
>>> Then you also don't need an assertion.  Thoughts?
>> Yeah, sounds good to be.  So long as I put a comment explaining that the default does the collocated Use* then i think it’ll be fine.
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>