[llvm-commits] [PATCH] Move stub allocation to the JITEmitter

[Jeffrey Yasskin]

On Tue, Nov 17, 2009 at 2:23 PM, Eric Christopher <echristo at apple.com> wrote:
>
> On Nov 16, 2009, at 4:07 PM, Jeffrey Yasskin wrote:
>
>> I've updated the patch to ToT at
>> http://codereview.appspot.com/download/issue153044_2015.diff.
>
> Few comments:
>
> +    struct StubLayout {
> +      size_t Size;
> +      size_t Alignment;
> +    };
>
>
> Comment above it giving whether it's in bytes, bits, etc?

Done.

> +TargetJITInfo::StubLayout PPCJITInfo::getStubLayout() {
> +  StubLayout Result = {10*4, 1};
> +  return Result;
> +}
>
>
> These could also use a block comment of the stub layout.
>
> I hate magic numbers. Not your fault that we have so many though :)

Documented. This led me to find a bug in how I'd changed
emitGlobalValueIndirectSym: ARM+PIC was using that inside
emitFunctionStub, which meant that it didn't work to move its stub
allocation to the JITEmitter. For now, I've rolled back that part of
the change. http://codereview.appspot.com/156053 will allow two
"stubs" to be active at once. We may want to rename the function that
allocates an indirect symbol, since it isn't a stub.

> +    /// for external functions.  TODO(jyasskin): Of course, external functions
>
>
> I think todos are for the readme file though you can reference that in your comment. Probably don't need your name though.

What readme? There isn't one in the ExecutionEngine tree, and the
top-level one doesn't have anything like this. I have deleted my name.
I can delete the TODOs entirely if you prefer, since I should be
fixing them soon.

> Otherwise it looks fine, I'll run it through tests here real fast.

http://codereview.appspot.com/153044 has the current patch + the
multiple-stubs fix. I'll merge once I commit that one.