[llvm-commits] [llvm] r76615 - in /llvm/trunk: include/llvm/CodeGen/LiveIntervalAnalysis.h lib/CodeGen/LiveIntervalAnalysis.cpp

[Chris Lattner]

On Jul 21, 2009, at 4:46 PM, David A. Greene wrote:
>>> +  // IntervalPrefixPrinter - Print live interval indices before  
>>> each
>>> +  // instruction.
>>> +  class IntervalPrefixPrinter : public PrefixPrinter {
>>
>> David, please talk about general infrastructure like this before just
>> applying it to mainline with no review.
>
> I thought the policy was "review after commit."

The intention of the policy is to streamline the normal case, but to  
let people use their discretion about when something might need  
additional review or discussion.  Technically, almost everyone should  
have their patches reviewed before applied.  In practice, this would  
slow things down a lot and waste a lot of time.


>> Remember that you have *commit after approval* access, with  
>> exceptions
>
> Really?
>
> "If you have recently been granted commit access, these policies  
> apply:"
>
> Is two years ago "recent?"

The wording there is written for people who are looking to get commit  
access.  It is not meant to imply that the rules expire over time.

>> for "obvious" patches.  This is not obvious and you are not a
>> maintainer for these areas.  Please read this for more information:
>> http://llvm.org/docs/DeveloperPolicy.html#commitaccess
>
...
>
> Ah, I misread this bit.  Still, I was surprised to learn I'm only  
> supposed
> to commit after getting everything reviewed.
> It's fine if this is the new policy, but having the goalposts moved  
> gets
> frustrating.

I really don't mean to "move the goal posts" on you, and I hope you  
know that I find your work and contributions to be extremely valuable.

> I will submit the revision for review before committing it.


These patches are a good candidate for prior review because the add  
significant new functionality: new files, new APIs and change what  
existing stuff is doing by generalizing things in major ways.  I  
reverted the patch instead of letting it go in and discussing it  
afterward because:

1) it is actively doing things that you know we don't like (e.g. using  
streams), making use of heavy weight classes like std::string, etc.
2) conflates a bunch of unrelated changes together: e.g. the  
'constifying' of Mi2IndexMap could be a separate and independent patch  
which would be "obvious".
3) Is a vastly over-general solution to a very simple and specific  
problem.

In addition, your last major patch (formatted ostream) required  
significant iteration because it was vastly over general, had several  
other major problems that were solved through review, and caused  
massive performance issues.  I haven't had a chance to review r76601  
yet, but I'm hoping that this resolves the remaining issues.

I was slightly over-generous when handling your recent tblgen patch.   
It got reverted due to correctness changes, but when Evan reported the  
obvious and measured perf problems, we technically should have  
reverted it again.  I didn't appreciate it when you reapplied the  
patch (with correctness fixes) even though there were large  
outstanding performance issues.  It seems that it would be more  
natural to fix all the problems found by review and the testers, then  
re-apply it.

In the end, I think this patch will be end up different enough that it  
should be discussed before it goes in.  Specifically for this patch,  
please remove all the generic "prefix handling" stuff and just make a  
change localized to LiveIntervalAnalysis routine.  The double-nested  
for-loop is not that much code to duplicate.


> I have a *lot* of stuff I'm planning to send.  To me, some of it will
> obviously need prior review.   This wasn't one of those cases.

Again, I want reemphasize that I value your work and your  
contributions.  I really am not trying to drive you away.  In order  
for the open source project to make progress, we have to balance speed  
of development with impact on the various developers.  For changes  
that add major new APIs, classes, and concepts, please discuss it  
ahead of time.  Trying to short-circuit the process won't make things  
go faster in the end.

>> Please revert r76602+ and propose them to llvm-commits.
>
> I see someone already did.  Would've been nice to get an e-mail  
> about it.

I assumed that you read commits, so you'd see it.

-Chris