[llvm-commits] [llvm] r71610 - in /llvm/trunk: include/llvm/CodeGen/MachineFunction.h include/llvm/Intrinsics.td lib/CodeGen/MachineFunction.cpp lib/CodeGen/PrologEpilogInserter.cpp lib/CodeGen/SelectionDAG/SelectionDAGBuild.cpp lib/Target/ARM/ARMISelLowering.cpp lib/Target/ARM/ARMISelLowering.h lib/Target/ARM/ARMInstrInfo.cpp lib/Target/ARM/ARMInstrInfo.td

[Chris Lattner]

On May 13, 2009, at 11:09 AM, Jim Grosbach wrote:
>> How about llvm.save.register.state() and  
>> llvm.restore.register.state().  LangRef should document that these  
>> are compatible with the GCC builtins.  Also, you should use  
>> GCCBuiltin<> in the .td file so that these work with the CBE etc.   
>> Please make sure to update 'HasBuiltinSetjmp' and the accessors  
>> when you rename the intrinsic.
>
> I didn't put in the GCCBuiltin<> reference because these intrinsics  
> are not exactly equivalent. The intent is to generate code that will  
> interoperate correctly with the GCC versions, but there are  
> differences in the implementation currently. I may be able to  
> reconcile the details, and if so, I'll add in the GCCBuiltin bits.

Ok.

>>> +++ llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp Tue May 12  
>>> 18:59:14 2009
>>> @@ -180,7 +180,7 @@
>>> std::vector<CalleeSavedInfo> CSI;
>>> for (unsigned i = 0; CSRegs[i]; ++i) {
>>>   unsigned Reg = CSRegs[i];
>>> -    if (Fn.getRegInfo().isPhysRegUsed(Reg)) {
>>> +    if (Fn.getRegInfo().isPhysRegUsed(Reg) ||  
>>> Fn.doesHaveBuiltinSetjmp()) {
>>
>> This is unfortunate, why does PEI need to know about this?   
>> Shouldn't the 'Int_builtin_setjmp' use and def all these registers?
>>
>
> It's the PEI that does the actual save/restore of the registers. The  
> intrinsic just tells the function that it's necessary. This allows  
> the actual jump buffer for the intrinsics to be very small (five  
> words) since the majority of the context save is on the stack, not  
> in the jump buffer.

Ok, that makes sense.  I'm not arguing about the generated code, I'd  
just like to get rid of the doesHaveBuiltinSetjmp() bool and special  
case in PEI.

> Callee-saved register values be preserved across function calls, and  
> inside our function, any values which are live at the time need to  
> be preserved across the setjmp() call itself. For the former, the  
> compiler already handles the requisite dirty work in the epilogue  
> and prologue code generation for any callee saved registers  
> potentially clobbered by the function. When a function contains a  
> builtin setjmp, we mark the function and consider all callee-saved  
> registers to be used in prologue/epiloge generation. For the latter,  
> we want to ensure that all values we care about are not stored in  
> registers across the setjmp, so the target implementation of the  
> intrinsic should mark it as defining all relevant registers (via  
> Defs = ...).

Ok, I understand how this works with your patch, but I still don't  
understand why it is needed.  Doesn't the machine instr that you  
insert end up using or def'ing these registers?  If so, isn't that  
already sufficient to get PEI to do the right thing without the bool?

>>> +let Properties = [IntrNoMem] in {
>>> +def int_builtinsetjmp  : Intrinsic<[llvm_i32_ty],  [llvm_ptr_ty]>;
>>> +def int_builtinlongjmp : Intrinsic<[llvm_void_ty], [llvm_ptr_ty,  
>>> llvm_i32_ty]>;
>>
>> If there is no reason to have both arguments, please drop the extra  
>> one.
>>
> I'm not sure I follow. builtinsetjmp takes a single argument, and  
> builtinlongjmp takes two. All arguments will be used.

Ok.

-Chris