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

[Chris Lattner]

On Jul 22, 2009, at 8:04 AM, David Greene wrote:
>>> 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
>
> Ok, I don't understand this.  What's "major" about it?  All of the
> change requests I've received have been easily fixable things.  I  
> don't
> think a reversion was necessary in this case.  It's not a huge deal to
> send it all again, but it is a bit frustrating when it's not clear  
> to me
> what the standard is.

At the end of the day, it's "major" when it is wrong for some reason,  
and disruptive for the tree while its in.  In this case, I don't like  
the design (a general form for prefixes is over-engineering a simple  
problem).  Many of your recent patches have had similar problems.  To  
avert this, you don't have to propose entire patches, just say "I want  
to add this thing, does this approach make sense" and a lot of  
confusion can be averted.

Not all of this is avoidable of course, but you already know (from  
previous exchanges) that we don't like std streams etc.  Committing  
something you know is "wrong" is not a good way for it to stick in the  
tree.

> Bottom line is the patch dumps some extra information in debug mode.
> Functionality-wise, that's not "major" to me.  It did change the  
> dump()
> API's for some classes but I tried to mitigate that with default
> arguments (not always an idea solution, I'll admit, and maybe not the
> right one here).

The reason it's wrong in this case is that a very pass-specific debug  
info dump is now pervasively affecting unrelated code.

>> 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.
>
> But I don't recall anyone saying std::string is forbidden.  Nor
> #include <sstream> and std::stringstream.  Those don't have the static
> constructor problems iostreams has, AFAIK.

I agree that they aren't as evil as <iostream>, and perf in debug mode  
doesn't really matter.  You're right.

>> 3) Is a vastly over-general solution to a very simple and specific
>> problem.
>
> This functionality will not only be used in LiveIntervals.  It will  
> also
> be used by the register allocator and the coalescer, at least.   
> Basically,
> almost anything in codegen after live interval analysis can use it.   
> Doesn't
> it make sense to factor the code out into a function object that can  
> be
> invoked from several places?

Perhaps, but it is non-obvious enough to me that it warrants  
discussion.  If you'd like to discuss it, lets do so, but not in this  
thread.  Please start a new one.

>> 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.
>
> What performance issues?  I don't remember seeing anything on that.   
> If
> so, I'm interested in fixing them.  Honestly, no one ever told me  
> about
> it.  It's very difficult to keep up with llvm-commits without being
> copied explicitly on important e-mails like this.

I believe that Evan cleared this up.

> I was also thinking along the lines of incremental changes.  Fix the
> correctness issue first (which, BTW, I couldn't possibly have known
> about given the testbase at the time) and then fix the remaining  
> issues
> after that.  That was a deliberate choice on my part to try to comply
> with developer policy.

I really appreciate the incrementalism!  In this case, it sounds like  
you missed an email about the perf issues, so it's just a  
communication issue.

>>>>
>>>> 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.
>
> I do read commits.  I can't possibly read all of the hundreds of
> messages that fly by every day.  I try to scan and pick out the ones
> that are most relevant to me.
>
> Actually, what would help here is a better post-commit hook that  
> allows
> us to specify a subject in the commit e-mail.  git does this in a nice
> way by assuming the log begins with a short line suitable for an e- 
> mail
> subject line followed by a longer descriptive paragraph.

It seems that llvm is getting significantly more active recently.   
There are a couple of solutions to this, and your idea is a good one.   
Long term, I'd like to split the main llvm svn repo into multiple  
smaller ones.  This would allow us to split the codegen stuff from the  
mid-level optimizer stuff, etc.  That way, each could have its own  
commit list etc.  It would probably make sense to split the poolalloc,  
llvm-gcc, gcc plugin stuff out to their own lists now.

-Chris