[PATCH] Add a pass for constructing gc.statepoint sequences w/explicit relocations

[Philip Reames]

On 02/13/2015 02:46 PM, Andrew Trick wrote:
>> On Feb 13, 2015, at 2:24 PM, Philip Reames <listmail at philipreames.com> wrote:
>>
>>
>> On 02/09/2015 11:25 PM, Andrew Trick wrote:
>>> I agree with Sanjoy's comments. I'm not sure why you're opposed to cleaning up the code before submitting it for review:
>>>
>>> - using LLVM's ADT containers
>>> - following LLVM naming conventions (violated in a few places).
>>> - following LLVM comment style!
>>>
>>> Is it becase you want more feedback on the algorithm itself first?
>> I wouldn't say I'm "opposed to cleaning up the code".  What I was trying to do is minimize the total amount of work I have to do to get this upstreamed, cleaned up according to LLVMs conventions, and merged back into my tree.  Every change I make to this diff is one more merge conflict I need to resolve when merging back and one more chance to screw up the merge.  As an example, I've fixed two distinct sets of bugs in the PlaceSafepoint pass in my tree due to merge conflicts.  Both were relatively minor, but still.
>>
>> The workflow I would have preferred would have been:
>> - Submit code more or less as is.
>> - Merge back into my tree
>> - Iterate in tree to clean up style violations.
>>
>> This workflow would have resulted in *substantially* less work for me and less chance of breakage overall. Given that I'm effectively the code owner of this area and have a record of contribution, I really don't think that was an unreasonable request.
>>
>> At this point, it is clear that this is not going to work.  Given that, I will clean up the diff and live with the merge conflicts and bugs that are going to result.  Fair warning, I don't advise anyone actually use the result for at least a week or two after submission.  It's going to take that long or longer for me to re-stablize and identify all the regressions.
> If I were you I’d probably cleanup the head of my local branch first, then artificially split that into pieces that can be separately reviewed/committed to LLVM.
>
> When I was working on LLVM features, I would usually have a git branch with lots of small commits. I would then shuffle them around and merge them in a way that seemed useful for review. But if there was much cleanup involved, that became too labor intensive, so I would just take the HEAD revision and strip features off into a few large commits.
>
*snip*
>>
>>> Once in trunk, if you want more feedback on certain aspects of the pass, whether design or implementation, you'll just have to be clear on what kind of review you want.
>> I'm really unclear what you're trying to say here.  LLVM has a pre-commit review policy for non-obvious commits.  This is clearly a non-obvious commit.  Thus, I asked for review.  Your comment about "kind of review" isn't parsing for me.
> I don’t understand the purpose of pre-commit review if you’re just going to say you’ll make any changes at some later date, post-commit.
>
> I’m fine doing post-commit review of this file. You are the code owner—and it can’t affect anyone else. Just tell us when you’re ready for feedback and what kind of changes you’re ready to make. No one wants to waste time nit-picking code that will be rewritten. If you just want early feedback on the high-level design, make that clear.
>
> Did you expect to get anything out of this review other than permission to checkin?
No.  While I am very happy to receive feedback and incorporate it, but - 
as I explicitly stated in my review request - I wanted to get this 
landed and then iterate in tree.  I was giving interested parties a 
chance to comment or object.

It sounds like you'd have preferred I just submitted the change, cleaned 
it up in tree, and *then* asked for a review.  Up until now, I had been 
under the impression that I needed, at minimum, a signoff on the 
original change with permission to iterate.  I asked for that.  Sorry 
for any confusion that resulted.

Now, moving forward, what do we do?  I see two options:
1) I check in a mildly cleaned up version on Tuesday and then follow on 
with additional cleanup patches.  We pretend this review thread never 
happened and I request a separate post commit review once the cleanup 
patches have landed.
2) I submit a cleaned up version of the current change, possibly after 
waiting for another review cycle.

I would strongly prefer the first option.  You previously seemed to 
indicate a preference for the later.  Do you object to me taking the 
first option?  I want to get this patch landed next week whatever I have 
to do to get that done.

Philip