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

[Ivan Krasin]

Hi Jacob,

On Mon, Aug 1, 2011 at 10:35 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>
> On Aug 1, 2011, at 2:13 PM, Ivan Krasin wrote:
>
> this patch fixes the FastISel stack allocation strategy issue described
> here:
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-July/041452.html
> The solution is to emit immediate int arguments just before the call
> (instead of spilling them on stack at the beginning of the function)
>
> Thanks for working on this, Ivan.
> I don't think there is any reason to special-case function arguments. It is
> just as bad when fast-isel hoists the immediate in 'x+1'.
> I would prefer an approach that handled all immediates.
Thanks, it's a good point. I have tried to use double arguments as
immediates as well, but FastISel fails on them.
I think it also should be fixed, but I would prefer to make it in the next CL.

>
> --- test/CodeGen/X86/fast-isel-call-x86-64.ll (revision 0)
> +++ test/CodeGen/X86/fast-isel-call-x86-64.ll (revision 0)
> @@ -0,0 +1,59 @@
> +; RUN: llc < %s -O0 -fast-isel-abort -march=x86-64 | FileCheck %s
> +; This test should be merged with fast-isel-call.ll,
> +; but currently it's not possible because FastISel lacks
> +; of llvm.memcpy and llvm.memset.
>
> Huh? None of the tests use memcpy or memset.
Sorry, it was a non-adequate comment. I have merged the test with
fast-isel-x86-64.ll and removed all non-related tests.

>
> +define i32 @test1() nounwind {
> +tak:
> + %tmp = call i1 @foo()
> + br i1 %tmp, label %BB1, label %BB2
> +BB1:
> + ret i32 1
> +BB2:
> + ret i32 0
> +; CHECK: test1:
> +; CHECK: callq
> +; CHECK-NEXT: testb $1
> +}
>
> Please add a comment describing what you are testing for here.
Deleted. Not related to the patch. Sorry.
>
> +define void @test2(%struct.s* %d) nounwind {
> +  call void @foo2(%struct.s* byval %d )
> +  ret void
> +
> +; CHECK: test2:
> +; CHECK: subq $24, %rsp
> +; CHECK: movq (%rdi), %rax
> +; CHECK: movq %rax, (%rsp)
> +; CHECK: movl 8(%rdi), %ecx
> +; CHECK: movl %ecx, 8(%rsp)
> +; CHECK: callq foo2
> +; CHECK: addq $24, %rsp
> +; CHECK: ret
> +}
>
> Same thing here. Also, is this related to your patch at all? Why must the
> byval argument be copied in that order and using exactly those temporary
> registers?
Removed.

> The last test looks good.
Please, take another look.
You can easily track what's changed from the last patch at
http://codereview.chromium.org/7539010/

Thanks,
Ivan Krasin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fastisel-krasin-p3.diff
Type: text/x-patch
Size: 3155 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110802/5d1c666d/attachment.bin>