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

[Daniel Dunbar]

On Thu, Mar 4, 2010 at 5:59 AM, John McCall <rjmccall at apple.com> wrote:
> 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.

We already do this for Type::getInt*Ty

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

Yeah, I understand, I just find stray utility functions clutter the
code more than they help.

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

Ok.

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

I think adding a target hook is the the right solution, at least for now.

 - Daniel

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