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

[Andrew Trick]

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

>> You say you plan "The liveness algorithm improvements mentioned previously." Do you just mean the improvements listed above in the commit message?
> Yes.
>> There isn't really any liveness analysis here at all. The LiveVariables approach is pretty good for SSA liveness, so you could just steal it. From your comments I gather your saving that as a follow up improvement.
> As I mentioned, I ripped out the liveness implementation we were using.  It's a mess and clearly not the right direction going forward.  I was unaware of LiveVariables, but will take a close look.
>> If you fix the obvious style issues, I don't really object to this being in trunk. We've already established that the pass will be on trunk (but not in the standard pipeline).
> To be clear, this is intended to be a LGTM contingent on the cleanup mention before submission correct?  Are you expecting another round of review before submission?
>> 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?

Andy

>> 
>> 
>> ================
>> Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:163
>> @@ +162,3 @@
>> +    for (Type *SubType : ST->subtypes())
>> +      bad |= isGCPointerType(SubType) || isAggWhichContainsGCPtrType(SubType);
>> +    return bad;
>> ----------------
>> This "bad" variable name is mysterious. In this scope, it doesn't seem "bad".
> Agreed.  Will fix.
>> 
>> http://reviews.llvm.org/D6975
>> 
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>> 
>> 
>