[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

[Jim Grosbach]

On May 13, 2009, at 11:21 AM, Chris Lattner wrote:

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

Ah, gotcha. I understand better what you're asking now. Sorry for the  
confusion.

Hmmm... I didn't see that happening when I was working through those  
bits before; however, I may not have had everything hooked up right  
yet at the time. I agree it would be far, far preferable to not need  
the "magic save everything" flag on the function. I'll play around  
with this some more and see what I can do.

Thanks again.

-Jim

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