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

[Chris Lattner]

On Feb 13, 2008, at 1:47 AM, Evan Cheng wrote:
>> Very nice, does it also help shootout/fib?
>
> bh speedup doesn't seem real. It doesn't help fib.

Ok, once things have settled, please take a look at the comments in  
the bugzilla to see if there are other cases being missed.

>> This idiom happens a lot in the preexisting code.  Please use:
>>
>> VNInfo *AValNo = IntA.FindLiveRangeContaining(CopyIdx-1)->valno;
>
> ALR is also used below.

Ok!

>>> +  for (unsigned i = 0, e = DefMI->getNumOperands(); i != e; ++i) {
>>
>> This loop needs a comment. What are you trying to find with this
>> loop?  Maybe it should be moved out to its own function which just
>> returns idx?  This would force you to name it, which would describe
>> what it does :)
>
> There is a reason I said "initial" in the commit message. This is not
> done. I need a target hook to tell me what is the new destination
> register after commuting. Right now this is basically assuming x86
> commutable instructions: A = A + B

Ok.

>> Another random question unrelated to this code: now that we have
>> efficient RAUW, can we eliminate the 'rep'  mapping stuff and just
>> RAUW vregs as they are coallesced?
>
> Well, actually it's related. I think it's required for correctness.
> See below.

Ah, I see.  Well this seems like an independently useful change that  
will both speed up and simplify coalescing.  Are you interested in  
tackling it as part of this project?

>> Yay for use_iterators :)
>
> Except I don't think this is quite right. Iterating through uses of
> IntA.reg means we end up not updating other uses that have already
> been coalesced to it.

Yeah, that's bad.

>> Where the use iterator could point to the first r2, and incrementing
>> it could point to the next r2.  This could be avoided by not zapping
>> instructions in this loop: move the copy zapping to a walk over a
>> smallset or something.
>
> Nothing is being zapped in the loop. Copies that would be zapped are
> put into a set JoinedCopies. In fact, all uses have to be updated to
> the new register.

Ok.

>> It isn't clear to me what this code is doing.  It looks like it is
>> looking for the copy that has now becomes an identity copy.  Is there
>> some way to factor this with other code that is coallescing copies
>> away?  It seems duplicated from somewhere else.  It would be nicer to
>
> Not really. A number of things are happening here. We are trying to
> determine which copies are now identity copies and make sure their
> valno# will be eliminated. We also need to transfer the live ranges
> defined by the (now dead) copies to the new val# defined by the
> commuted definition MI. It's also tracking kill information.

Ok, please comment it a bit better.  Is there any way to avoid this  
work or factor this out into functions that can be shared with other  
code?

>> just add all copies to a small vector and then zap them later or
>> something, to keep the logic simple.
>
> That's essentially what's happening. This has gone through a number of
> iterations already. Apart from some cosmetic changes, it's not going
> to get much simpler than this.

ok.

>> Does this need to be an ivar?  Can it just be passed by-ref between
>> the methods that need it?
>
> This will be cleaned up later as part of RAUW change.

Ok.  Thanks Evan!

-Chris