[cfe-commits] r97653 - /cfe/trunk/lib/CodeGen/CGBuiltin.cpp

[John McCall]

On Mar 4, 2010, at 5:41 AM, Daniel Dunbar wrote:
> On Wed, Mar 3, 2010 at 2:30 AM, John McCall <rjmccall at apple.com> wrote:
>> +static llvm::ConstantInt *getInt32(llvm::LLVMContext &Context, int32_t Value) {
>> +  return llvm::ConstantInt::get(llvm::Type::getInt32Ty(Context), Value);
>> +}
> 
> I would prefer not to have a one off utility like this dangling in
> CGBuiltin. If we want such a thing, we should consistently use it.
> 
> Would it make sense to make this a method on llvm::ConstantInt?

The trouble is that yes, people want these, but they want them at different widths.  I think we probably don't want all of getInt8(), getInt16(), getInt32(), and get64().  Maybe we do.

I did consider pushing this to CodeGenFunction; I decided not to, because most of codegen doesn't make constants of constant width.  That said, there are probably other places that would use it.

>>  RValue CodeGenFunction::EmitBuiltinExpr(const FunctionDecl *FD,
>>                                         unsigned BuiltinID, const CallExpr *E) {
>>   // See if we can constant fold this builtin.  If so, don't emit it at all.
>> @@ -343,6 +347,20 @@
>>                         llvm::ConstantInt::get(llvm::Type::getInt32Ty(VMContext), 1));
>>     return RValue::get(Address);
>>   }
>> +  case Builtin::BI__builtin_dwarf_cfa: {
>> +    // The offset in bytes from the first argument to the CFA.
>> +    //
>> +    // Why on earth is this in the frontend?  Is there any reason at
>> +    // all that the backend can't reasonably determine this while
>> +    // lowering llvm.eh.dwarf.cfa()?
>> +    //
>> +    // TODO: If there's a satisfactory reason, add a target hook for
>> +    // this instead of hard-coding 0, which is correct for most targets.
> 
> Please use FIXME instead of TODO for easier grepability.

I don't consider these interchangeable.  We've also got hundreds of TODOs already — an order of magnitude fewer than we have FIXMEs, but still, they're not rare.

> Is this wrong
> for targets we actually have support for? If so, it seems like we
> should go ahead and add the hook and fix it.

What is actual support?  It's not wrong for the 3? 4? platforms I bothered to track down DWARF documentation for.  The only reason I haven't introduced the hook already is because I'm not sure that's the right solution, as noted by the comment above, which is a question I am actually going to ask people about when I am around the office tomorrow. :)

John.