[RFC] Remove User::OperandList
Pete Cooper
peter_cooper at apple.com
Tue Jun 9 22:38:11 PDT 2015
> On Jun 9, 2015, at 9:29 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>
>> 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).
Good point. I’ll make sure to do that.
>
>> 0001 - unchanged
>
>> + return User::allocHungoffUses(N, true);
>
> Please add a comment like:
>
> return User::allocHungoffUses(N, /* IsPhi */ true);
>
> LGTM with that.
Sounds good.
>
>> 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.
Yeah, didn’t think to do that. I’ll add that.
>
> 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.
I’ll take a look at the IR I think, or perhaps what gets inlined. If its able to be optimized i’ll leave it as is, otherwise will move it.
>
>> 0004 - Renamed reallocHungOffUses to growHungoffUses
>
>> + growHungoffUses(ReservedSpace, true);
>
> Please add a comment like:
>
> growHungoffUses(ReservedSpace, /* IsPhi */ true);
Yep, will do.
>
> 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.
Yeah, i should have explained this one better. Ultimately i was trying to move all the hung off subclasses to the new version of operator new without any other changes in behavior. Given that they all used to call new(size, 0), i made this version of new initially do exactly that and have the 0 too.
This version of new does set the HasHungOffUses boolean to true so as to allow the asserts that we only use the hung off uses methods when allowed.
Would you prefer I squash it, improve the commit comments, or leave it as is given the explanation here? I can also elaborate more if i didn’t explain it well.
>
>> +}
>> +
>>
>
>> 0007 - unchanged
>
> Except for the changes that depend on 0006 (and I think these can just
> be reordered?), this LGTM.
Yeah, I’ll take a look a what patches can be reordered appropriately. 0009 for example should really be before 0008 so that 0008 is the last commit and the one that actually changes the underlying structure layout.
Thanks for all the reviews here. I’ll update everything with clang-format and commit anything with a LGTM before 0006. Will wait on anything from 0006 onwards to see what you think there.
Thanks,
Pete
>
>> 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
>>
>
More information about the llvm-commits
mailing list