[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

Evan Cheng evan.cheng at apple.com
Wed Aug 12 18:12:02 PDT 2009


On Aug 12, 2009, at 12:24 PM, Jakob Stoklund Olesen wrote:

>
> 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.

Right.

>
>> 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.

If the target that uses RS wants to be smart, then it should something  
like what ARM is doing. To me, RS's job is not that. It should just  
make sure it's correct. That means either a target callback or  
returning a set of CSRs which need to be spilled. The client should be  
responsible for figuring out the best way to handle the information.

>
>>>> 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.

I agree there is a fine distinction between all these different ways  
of looking at unused CSRs. But the fact is we don't know what's in  
these registers. It could be the caller doesn't actually care about  
these CSRs. When RS is saving a CSR, I'd suggest just mark the  
register undef to play nice with liveness computation. It's ok for  
machine verifier to treat them as MBB liveins if you want to catch  
this class of bugs.

>
> 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.

Right. I think it's a fixable problem. Basically, this means these  
CSRs can only be used in certain blocks. For now, we can just say  
shrink wrapping is only useable if RS is not used by the target. John,  
what do you think?

Evan

>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list