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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
