[llvm-commits] [llvm] r78650 - in /llvm/trunk: include/llvm/CodeGen/RegisterScavenging.h lib/CodeGen/RegisterScavenging.cpp test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll

[Jakob Stoklund Olesen]

On 12/08/2009, at 20.21, Evan Cheng wrote:

>
> On Aug 12, 2009, at 12:04 AM, Jakob Stoklund Olesen wrote:
>>>> That can happen for one of two reasons: 1) The CSR has been saved  
>>>> in
>>>> the prologue and happens to be unused at this point, or 2) The CSR
>>>> has
>>>> not been spilled because it is not used in the function.
>>>
>>> Ok. There is sufficient information to distinguish the two cases,
>>> right?
>>
>> Not really. The current code is quite fragile but errs on the safe
>> side. It only clobbers the CSR if it isn't used any more in the MBB.
>>
>> We could use CalleeSavedInfo, but that is not very accurate when
>> shrink wrapping is used.
>>
>> We can detect 3) PEI has not inserted CSR spills yet. Then
>> CalleeSavedInfo will be empty.
>
> Let's ignore shrink wrapping for now.

OK. In that case, CalleeSavedInfo is all we need.

> We just need to know whether a
> register has been used (MachineRegisterInfo tracks this) and whether
> RS is being called before CSR spilling.

Yes. We can assume that isPhysRegUsed() accurately reflects the  
spilled registers.

>
>>
>>>> In the first case, the CSR can be used directly without being  
>>>> saved.
>>>> In the second case, it must be saved. If the CSR is used later in
>>>> the
>>>> MBB, the scavenger assumes the first case. If it isn't used anymore
>>>> in
>>>> the MBB, we assume the second case and mark the CSR live before
>>>> spilling it.
>>>
>>> My preference is to let the client of the scavenger to be  
>>> responsible
>>> for handling these details.
>>
>> I am not sure that is a good idea. We are distributing the handling  
>> of
>> CSRs among three parties: PEI, scavenger, and the target.
>>
>> Blackfin uses this code whenever it needs a register:
>>
>>  unsigned Reg = RS->FindUnusedReg(RC, true);
>>  if (Reg == 0)
>>    Reg = RS->scavengeRegister(RC, II, SPAdj);
>>
>> It is completely clueless, it just wants a register. Only ARM seems  
>> to
>> do something different.
>>
>> I would prefer to let PEI provide accurate live-in lists, let the
>> scavenger track available registers accurately based on that info,  
>> and
>> let the client simply ask for a register without worrying about which
>> CSRs are spilled at this exact instance.
>
> You mean RS should treat unused CSRs as if they are defined and livein
> to every MBB. Then RS is responsible for spilling and storing them?
> That's ok. But please don't add them to MBB liveins. The meanings of
> the two are completely different.

OK, we can track them separately.

The important thing is that the scavenger tracks them, so it doesn't  
accidentally clobber a pristine CSR.

> But you do see this results in poor code quality, right? We'll end up
> spilling and restoring unused CSRs many times rather than just
> spilling and restoring them in prologue and epilogue.

Yes, I understand. I am going for correct first and good second ;-)

ARM currently tries to predict RS usage and make PEI spill an extra  
register. That is one approach. Another is a target callback to amend  
the prologue. A third would be to rerun PEI somehow. I am not sure  
which is better.

>>> We shouldn't be introducing them as livein to MBBs. Do you mean the
>>> scavenger should treat them as if they are livein?
>>
>> Actually, I want to add them to the live-in lists. That way, the
>> machine code verifier will also be able to detect clobbered CSRs.
>
> That doesn't make sense to me. Why should unused CSRs be considered
> livein to MBBs by the machine code verifier? Nothing in the code
> should use undefined registers (unless it's explicitly marked undef).

I guess it depends on how you think about a live register. You think  
it contains a usable value. I think it contains a precious value that  
must not be clobbered. In reality there are three options:

1. Usable and precious (live)
2. Useless and garbage (dead)
3. Useless but precious (pristine CSR)

Perhaps the verifier should use this three-state model rather than the  
current dead/alive model.

>> PEI is already doing this with spilled CSRs, is there a downside to
>> doing it?
>
> PEI is not doing this right. The spilled register operand should be
> marked "undef" since there wasn't a prior def.

Undef is not quite right. An undef operand means "any register of the  
right class, don't care". Here it is very important to use the right  
register. You could say that "undef" is the fourth register state:

4. Usable but garbage (undef)

This is interesting. We have two dimensions: useless/usable and  
precious/garbage. The scavenger cares about the precious/garbage axis  
because it wants to avoid clobbering anything precious. The verifier  
should probably care about both.

You say "Let's ignore shrink wrapping". Can I break it even more? If I  
allow the scavenger to use spilled CSRs without proper per-MBB CSR  
tracking, it can lead to clobbered CSRs under shrink wrapping. ARM  
does that today, I think.