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

[David Greene]

Chris Lattner wrote:

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

Ok.  That could use a bit of rewording then.  I can see how it could be
interpreted both ways.

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

Thanks.  That's really not what this is about though.  Getting better
clarity on guidelines and procedures is always a good idea.

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

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

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

Remember, this happens in *debug* mode.  Returning a std::string isn't
very heavyweight when you're dumping lots of gunk to stderr.  :)

But I've changed that along the lines of what Bill requested and I have
no problem doing so.  It'll be better in the long run.

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

No, not really.  Not if you want const-correctness.  Believe me, I tried
to separate them.  I suppose I should have submitted the constifying
patch earlier.  Noted.

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

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

Again, I didn't see the report.  Can someone send it to me?

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

How could I fix a performance issue I didn't know about?  I see people 
committing stuff with "Make changes requested by XYZ" all the time.

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.

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

Again, it's a matter of opinion.  I like more general solutions, you
appreciate specific ones.  I don't think there's anything fundamentally
wrong with either.  Of course I'll make the changes requested and send
the patch for review ahead of time.

In this particular case, I don't see how I can make a change local to 
LiveIntervalAnalysis when the dump functions are in MachineFunction and 
MachineBasicBlock and are reused all over the place.  Maybe I'm missing 
something.  Do you have a particular solution in mind?

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

I certainly wasn't trying to short-circuit anything.  I honestly didn't
think enhancing debug dumps would be a huge deal.  I understand better
what the goal is now and that will help.

>>> 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 would really help with issues like this.

Thanks for discussing this.

                            -Dave