review (S) for 6862863: C2 compiler fails in elide_copy()

[Tom Rodriguez]

On Aug 12, 2009, at 12:11 PM, Vladimir Kozlov wrote:

> Tom Rodriguez wrote:
>> On Aug 11, 2009, at 9:49 PM, Vladimir Kozlov wrote:
>>> Why yank_if_dead() checks only in(1) if it is also dead
>>> and not other inputs which could be dead also after old
>>> node is removed? should we fix it?
>> It should never have more than 2 inputs and the control input is  
>> irrelevant.  It's either a MachSpillCopy or a Con of some sort.   
>> Maybe there should be an assert for no more than 2 inputs?
>
> The assert would be nice.

I've added it.

>
>>> Also from the current code it is not clear that at the end
>>> it does next for this nreg:
>>>
>>>             regnd.map(nreg,n);
>>>             value.map(nreg,val);
>>>
>>> (and corresponding nreg_lo mapping for register pair).
>>> May be it needs additional comment in your changes.
>> It's got a comment to that effect already.  What kind of comment  
>> were you thinking?
>
> Something like this:
>
> // Clear out a dead value mapped by the register to update
> // the register->node mapping even if 'n' defines the same value.

Where do you want this comment?

>>> Also in the new method you use "regnd->at(nreg)"
>>> when in other places regnd and value are passed as
>>> references and "regnd[nreg]". Can you do the same?
>> The code uses a mix of NOde_List* and Node_List& which is pretty  
>> ugly.  I'd copied the signature of yank_if_dead which was already  
>> using Node_List*.  I can change it to Node_List& if you like it  
>> better.
>
> yank_if_dead() uses (*regnd)[old_reg]
> I agree that it may be ugly but consistent.

I switched it to use Node_List&.

tom

>
> Vladimir
>
>> tom
>>>
>>>
>>> Thanks,
>>> Vladimir