[llvm-commits] [llvm] r44300 - in /llvm/trunk: include/llvm/Target/TargetLowering.h lib/CodeGen/SelectionDAG/LegalizeDAG.cpp lib/CodeGen/SelectionDAG/LegalizeDAGTypes.cpp lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp lib/Target/ARM/ARMISelLowering.cpp lib/Target/ARM/ARMISelLowering.h lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86ISelLowering.h

[Chris Lattner]

On Nov 24, 2007, at 12:55 AM, Duncan Sands wrote:

> Hi Chris,
>
>> /// RemapNode - If the specified value was already legalized to  
>> another value,
>> /// replace it by that value.
>> void DAGTypeLegalizer::RemapNode(SDOperand &N) {
>> -  DenseMap<SDOperand, SDOperand>::iterator I =  
>> ReplacedNodes.find(N);
>> -  if (I != ReplacedNodes.end()) {
>> -    RemapNode(I->second);
>> +  for (DenseMap<SDOperand, SDOperand>::iterator I =  
>> ReplacedNodes.find(N);
>> +       I != ReplacedNodes.end(); I = ReplacedNodes.find(N))
>>     N = I->second;
>> -  }
>> }
>
> in the original version this updated the map too (or at least it was  
> supposed
> to).  For example, suppose we remap N and the map contained:
> 	N -> A
> 	A -> B
> 	B -> C
> then afterwards we would get N = C, but also the map would be  
> modified to:
> 	N -> C
> 	A -> C
> 	B -> C
> This was supposed to speed up later lookups.

Urg, I completely missed that.  Restored, sorry!

>> +    if (SDNode *P = TLI.ExpandOperationResult(N, DAG)) {
>> +      // Everything that once used N now uses P.  P had better not  
>> require
>> +      // custom expansion.
>
> How about an assertion that it doesn't require custom expansion?

I don't know what I was thinking with that comment, I updated it to  
better describe the constraints.

Thanks for the review Duncan!

-Chris