[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 12, 2009, at 11:36 PM, Chris Lattner wrote:

> On May 12, 2009, at 4:59 PM, Jim Grosbach wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=71610&view=rev
>> Log:
>> Add support for GCC compatible builtin setjmp and longjmp  
>> intrinsics. This is
>> a supporting preliminary patch for GCC-compatible SjLJ exception  
>> handling. Note that these intrinsics are not designed to be invoked  
>> directly by the user, but
>> rather used by the front-end as target hooks for exception handling.
>
> All new non-target-specific intrinsics need to be documented in  
> LangRef.

Drat. I knew I'd forget something. Quite right. I'll take care of that.

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

>
>> +  // HasBuiltinSetjmp - true if the function uses builtin_setjmp.  
>> Used to
>> +  // adjust callee-saved register tracking.
>> +  bool HasBuiltinSetjmp;
> ...
>> +++ 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.

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

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

>> // 
>> = 
>> = 
>> = 
>> ----------------------------------------------------------------------= 
>> ==//
>> +// SJLJ Exception handling intrinsics
>> +//   setjmp() is a three instruction sequence to store the return  
>> address
>
> This is *not* setjmp and longjmp.  Please be very precise here.
>

Yep. I'll clean that up.

Thanks for the feedback!

-Jim