[llvm-commits] [llvm] r98007 - /llvm/trunk/lib/CodeGen/MachineCSE.cpp

[Evan Cheng]

On Mar 8, 2010, at 7:29 PM, Jakob Stoklund Olesen wrote:

> 
> On Mar 8, 2010, at 7:17 PM, Evan Cheng wrote:
> 
>> 
>> On Mar 8, 2010, at 7:14 PM, Jakob Stoklund Olesen wrote:
>> 
>>> 
>>> On Mar 8, 2010, at 6:53 PM, Evan Cheng wrote:
>>> 
>>>> 
>>>> On Mar 8, 2010, at 5:23 PM, Evan Cheng wrote:
>>>> 
>>>>> 
>>>>> On Mar 8, 2010, at 4:57 PM, Jakob Stoklund Olesen wrote:
>>>>> 
>>>>>> 
>>>>>> You could add "|| MRI->getRegClass(Reg)->hasSubClass(MRI->getRegClass(SrcReg))" without getting too complicated. That could help a bit with the few x86_64 subclasses as well.
>>>>>> 
>>>>> 
>>>>> I don't think it's necessary here. It's doing local coalescing here to avoid missing some obvious cse opportunities. If sub-reg class is involved, then it's already too complicated.
>>>> 
>>>> Actually I do need to make it more complicated. But then BlackFin hates me (I wonder if it's related to its author).
>>> 
>>> Hehe. Blackfin not hating you is a good sign that you got your register classes right ;-)
>>> 
>>> Basic register class algebra: If an instruction operand specifies register class X, a virtual register must belong to X or a subclass of X.
>>> 
>>> If you want to eliminate a copy "%x = mov %y", the resulting register must make the users of %x and %y happy at the same time. That means the new register must belong to the intersection of X and Y, the register classes of %x and %y.
>>> 
>>> The function getCommonSubClass(X, Y) finds the largest existing register class that is a subclass of both X and Y. If getCommonSubClass returns NULL, the classes are disjoint, and you cannot eliminate the copy - no register would be legal.
>>> 
>>> I think Blackfin is the only target with disjoint register classes - maybe that is why it hates you.
>> 
>> I don't think that's the issue here. For example, 2009-08-15-LiveIn-SubReg.ll:
>> Coalescing: %reg1031<def> = MOVE %reg1028
>> *** to: %reg1030<def> = SETEQdd %reg1027, %reg1031
>> 
>> *** Bad machine code: Illegal virtual register for instruction ***
>> - function:    foo
>> - basic block: entry 0x2036ee4 (BB#0)
>> - instruction: %reg1030<def> = SETEQdd %reg1027, %reg1028
>> - operand 2:   %reg1028
>> Expected a D register, but got a GR register
> 
> Sure. D is a subclass of GR.
> 
> %reg1028 is GR
> %reg1031 is D
> 
> When you coalesce the resulting register class should be getCommonSubClass(GR, D) = D, but you are using %reg1028 which is GR - too big.
> 
> The copy was probably created by InstrEmitter::AddRegisterOperand to make the register classes match.
> 
>> I'm going to commit my changes to machine cse (which is not turned on yet). Can you take a look at it? Thanks.
> 
> Sure.

Ok, I think the issue is more subtle. Your patch disabled some critical coalescing that's required. It probably should be allowed but then changes the register class.

Evan

> 
>