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

[Philip Reames]

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