[llvm-commits] [Review request] test/CodeGen/X86: Add cases for Win64 to 67 tests

[NAKAMURA Takumi]

Good midnight.
Michael, thank you to comment.

>> * 0001-test-CodeGen-X86-fold-mul-lohi.ll-FileCheck-ize-and-.patch
>>
>>  Win64 tends to emit "lea (%rip)". I excluded it from win64 tests.
>
> I think this is correct due to how Windows does PIC, but I'm not
> totally sure, as I'm not sure what code was generated on Linux before
> r43230.

I could make this win64-aware, to detect only lea (%rip), though,
I thought it might not be a test for win64.

>> * 0002-test-CodeGen-X86-red-zone.ll-Add-explicit-mtriple-x8.patch
>>
>>  Win64 doesn't use redzone stuff.
>
> Looks good to me.

When win64 codegen would become more efficiently, I could add a pattern
"w/o noredzone, similar sequences would be emitted on win64".

>> * 0003-test-CodeGen-X86-sse_reload_fold.ll-FileCheck-ize-an.patch
>>
>>  I don't understand what it intends. I am happy if someone explain to me. :)
>
> Looks good, except |& can be changed to |. I don't really know what
> this is testing.

It catches llc's diagnostic messages to stderr. The original test does, too.

>> * 0004-test-CodeGen-X86-store_op_load_fold2.ll-Add-a-test-f.patch
>>
>>  Alignment of 13th member in the struct makes difference.
>
> I would feel better if I actually knew what was being tested, but if
> the index is indeed significant, this looks good.

I will wait for 3rd opinion.

>> * 0005-test-CodeGen-X86-v2f32.ll-Fix-missing-tests-and-add-.patch
>>
>>  I re-enabled 3 tests and added *as-is* Win64's patterns.
>
> This looks correct to me. Stupid Windows x86-64 only using 4 GPRs for
> argument passing :(. Although I think we can get rid of the shadow
> stack because this is a leaf function.

In general, ToT win64 codegen tends to emit redundant stack operations
even for leaf functions. I think it would not be needed.
Please tell me its background, anyone.

>> * 0006-test-CodeGen-X86-Add-the-as-is-pattern-for-Win64.patch
>>
>>  I added *as-is* patterns for Win64. I intend for reviewrs to help to
>> reduce their expressions.
>
> This one is going to take a while to review :P.

Please try to rewrite and post them! :p
Or improve win64 codegen!

>> * 0007-test-CodeGen-X86-Mark-as-XFAIL-mingw-win32.-They-sho.patch
>>
>>  I thought they could be fixed in future. When one were fixed, "llc
>> -mtriple=x86_64-win32" should be added.
>
> I noticed you documented some of these. Can you document the rest and
> please not only reference PR numbers?

I will do and I will file a few related issues.

>> * 0008-test-CodeGen-X86-FileCheck-ize-with-relaxed-expressi.patch
>>
>>  Please watch out;
>>    - Is my FileCheck-izing reasonable?
>>    - Do relaxed patterns have any faults?
>
> diff --git a/test/CodeGen/X86/2007-01-08-X86-64-Pointer.ll
> b/test/CodeGen/X86/2007-01-08-X86-64-Pointer.ll
> index de226a1..90e7fab 100644
> --- a/test/CodeGen/X86/2007-01-08-X86-64-Pointer.ll
> +++ b/test/CodeGen/X86/2007-01-08-X86-64-Pointer.ll
> @@ -1,5 +1,8 @@
> -; RUN: llc %s -o - -march=x86-64 | grep {(%rdi,%rax,8)}
> -; RUN: llc %s -o - -march=x86-64 | not grep {addq.*8}
> +; RUN: llc < %s -mtriple=x86_64-linux | FileCheck %s
> +; RUN: llc < %s -mtriple=x86_64-win32 | FileCheck %s
> +; CHECK-NOT: {{addq.*8}}
> +; CHECK:     ({{%rdi,%rax|%rcx,%rax}},8)
> +; CHECK-NOT: {{addq.*8}}
>
> The patterns can be changed to.
>
> +; CHECK-NOT: addq{{.*}}8
> +; CHECK:     (%r{{di|cx}},%rax,8)
> +; CHECK-NOT: addq{{.*}}8
>
> A bunch of others are like this too. Put the minimum amount of text in
> the brackets.

I think it might be one's preference in style.
For human's readability, I took;
  - {{regex}} may be widened if can.
  - If a pattern contains metachars for regex, it might be tightened.

chapuni> +; CHECK:     ({{%rdi,%rax|%rcx,%rax}},8)
Anyway, this would be overkill. I prefer;
chapuni> +; CHECK:     ({{%rdi|%rcx}},%rax,8)
I feel your suggestion would be overkill, too.
bigcheese> +; CHECK:     (%r{{di|cx}},%rax,8)

> Only Linux and Windows are handled, not Darwin. I agree that all
> testers should test code gen for more than just the host platform, but
> we need to not exclude major platforms.

I suppose x86_64-darwin uses AMD64 ABI.
May I add 3rd action "llc -mtriple=x86_64(-apple)-darwin"?
I doubt it would be redundant in most cases.

> Other than that it looks fine, but I'm not sure if they are only
> testing what the author wanted. I really wish people would document
> their tests.

Me too, sometimes I would like to know what each test intends.
I expect every developer can improve tests occasionally.

>> * 0009-test-CodeGen-X86-FileCheck-ize-and-add-the-case-for-.patch
>>
>>  I added "WIN64:"(and similar) expressions to them.
>>  I wonder it might be reasonable.
>
> Adding WIN64 is OK if the test actually relies on how arguments are
> passed or other ABI issues. Other than that, same comments as above.

I added "WIN64" pattern when relaxed expressions would not match to one.
Most of them must be due to ABI difference.

> Thanks again for doing this. All these tests now pass for me on MSVC
> 2010 32bit and MinGW32 on Windows 7 x64.

I believe I could silence all test/CodeGen/X86 :)


Thank you, ...Takumi