On Thu, Mar 4, 2010 at 5:59 AM, John McCall <[email protected]> wrote:
> On Mar 4, 2010, at 5:41 AM, Daniel Dunbar wrote:
>> On Wed, Mar 3, 2010 at 2:30 AM, John McCall <[email protected]> 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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
