[llvm-commits] X86 FastISel: Emit immediate call arguments locally to save stack size when compiling with -O0

[Ivan Krasin]

Eric,

I have added comments before declarations of LastLocalValue and EmitStartPt.
Please find the patch attached or http://codereview.chromium.org/7612002/

Ivan Krasin

On Thu, Aug 18, 2011 at 1:14 PM, Eric Christopher <echristo at apple.com> wrote:
> :) Almost there.
>
> Can you describe the full algorithm in the few lines before the declaration of LastLocalValue and EmitStartPt?
>
> The rationale for the built-in is fine.
>
> -eric
>
> On Aug 18, 2011, at 1:12 PM, Ivan Krasin wrote:
>
>> Friendly ping :)
>>
>> On Tue, Aug 16, 2011 at 12:14 PM, Ivan Krasin <krasin at google.com> wrote:
>>> Eric,
>>>
>>> could you please take another look?
>>>
>>> Ivan
>>>
>>> On Mon, Aug 15, 2011 at 2:29 PM, Ivan Krasin <krasin at google.com> wrote:
>>>> On Thu, Aug 11, 2011 at 3:20 PM, Eric Christopher <echristo at apple.com> wrote:
>>>>>
>>>>> On Aug 10, 2011, at 4:01 PM, Ivan Krasin wrote:
>>>>>
>>>>> <flush-local-value-map.patch>
>>>>>
>>>>> In general I think the patch is OK.  A few requests:
>>>>>    MachineInstr *LastLocalValue;
>>>>> +  MachineInstr *OrigLocalValue;
>>>>> ….
>>>>>    // Start out as null, meaining no local-value instructions have
>>>>>    // been emitted.
>>>>> -  LastLocalValue = 0;
>>>>> +  OrigLocalValue = 0;
>>>>> A lot of the code you've changed hasn't had any comments written or updated
>>>>> for the new behavior. It'd be good to get an updated description of how the
>>>>> LocalValueMap is working and how it interacts with constants.
>>>> I have renamed OrigLocalValue to EmitStartPt which is (I believe) less
>>>> confusing naming, since it points to the place in the block where it's
>>>> allowed to start emitting instructions.
>>>>
>>>>> In particular:
>>>>> +  if (!isa<IntrinsicInst>(F))
>>>>> +    flushLocalValueMap();
>>>>> +
>>>>> Here. The location of this here doesn't make a whole lot of sense and it'd
>>>>> be good if you could explain it.
>>>> I've added the explanation. Thanks for the suggestion.
>>>>
>>>>> A better way to do this would be, as Jakob suggested, use the LocalValueMap
>>>>> as a storage for constants and locations that you've used them in the block
>>>>> and then emit all of the constants at that point rather than this weird
>>>>> flushing mechanism.
>>>> I've tried to do that. It slows down -O0 build which is unacceptable.
>>>> The problem is that we have to emit all local values to the start of
>>>> the block, store last use for each of them and spread them through the
>>>> block once the processing of the block is done. It makes it slower...
>>>> So, I would prefer to stay with the patch that makes all the metrics
>>>> slightly better on average (code size/stack size/compile time). Is it
>>>> fine with you?
>>>>
>>>> Please, find the updated patch attached.
>>>>
>>>> Ivan Krasin
>>>>
>>>>> -eric
>>>>
>>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: issue7612002_7001.patch
Type: text/x-patch
Size: 5142 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110818/003ba576/attachment.bin>