FileCheck-ize some tests in test/CodeGen/X86/

[jgorbe]

Summary

FileCheck-ize some tests in test/CodeGen/X86/

[jgorbe]

Forgot to add llvm-commits, commenting so a message gets posted there. Sorry!

[chandlerc]

In D29807#673088, @jgorbe wrote:

Forgot to add llvm-commits, commenting so a message gets posted there. Sorry!

Sure, looking at this now.

Just as an FYI for the future, it is a bit nicer to just abandon a revision when you miss llvm-commits at first, and post a fresh one. That way Phabricator attaches a patch file for folks who want to do the code review via email.

[jgorbe]

Noted, thanks!

[chandlerc]

Cool! More detailed comments than last time, some of which is just teaching you some of the basics of modern test hygene here. Some high level comments:

• You may want to strip off the boilerplate comments added by LLVM like the type and number of uses. Those tend to just make it hard to edit and/or scan the code without adding value.
• You may want to re-indent/format the test code by running it through the opt tool or doing llvm-as | llvm-dis on it. Note that this will add back comments that you will then want to strip off.
• I would encourage you to add names for basic blocks where they are missing as it makes it easier (IMO) to read and update the test. It also makes debugging some test failure error messages easier. I usually just start with entry:.

You don't always need to do this when switching a test from grep -> FileCheck, but it is often nice. Especially if the test is quite small, I think it is a good practice to tidy it up while there.

See some more specific comments about these tests below.

Thanks again for working on this!

test/CodeGen/X86/2003-11-03-GlobalBool.ll

2 ↗

(On Diff #87941)

You can use FileCheck's patterns for this:

; CHECK-NOT: .byte true

The whitespace is already not significant in FileCheck.

Also, I'd put this just after the global declaration below.

test/CodeGen/X86/2004-02-13-FrameReturnAddress.ll

10–11 ↗

(On Diff #87941)

Generally you should put the -LABEL right at the top of the area you want to test, and you should make sure it won't spurriously match other strings. For example:

define i8* @test1() {
; CHECK-LABEL: define i8* @test1()

Also, you'll want to look at what 'llc' produces here and try to understand what the grep pattern above is really looking for (%esp seems to miss a lot of context, and now that this test is using FileCheck we want to try to get a reasonably good guess at what the original test was trying to detect.

test/CodeGen/X86/2004-02-14-InefficientStackPointer.ll

5–6 ↗

(On Diff #87941)

You should be able to add a triple to pin down the particular assembly format and then look for sub %esp without having to use a regular expresssion.

test/CodeGen/X86/2006-01-19-ISelFoldingBug.ll

18–20 ↗

(On Diff #87941)

For tests like this (very small, testing a very *specific* and short sequence of instructions) I would encourage you to switch them to FileCheck and use llvm/utils/update_llc_checks.py to generate precise and clean checks for them.

[jgorbe]

Made some changes suggested by chandlerc:

• Removed boilerplate comments added by LLVM: types and number of uses
• Reindented test code to match the output of opt
• Added 'entry' labels to basic blocks
• Changed 2006-01-19-ISelFoldingBug.ll to a version generated by llvm/utils/update_llc_checks.py
• Removed a couple of unnecessary regular expressions
• Changed some test CHECKs to be more accurate than the existing grep search strings.

[jgorbe]

Some responses to @chandlerc 's previous comments.

test/CodeGen/X86/2003-11-03-GlobalBool.ll

2 ↗

(On Diff #87941)

Done, thanks!

test/CodeGen/X86/2004-02-13-FrameReturnAddress.ll

10–11 ↗

(On Diff #87941)

llc won't output "define i8* @test1()" so that particular example wouldn't work here, right? I added a colon to match "test1:" as in the actual x86 assembly label.

As for the pattern matching (%esp, I preferred to err on the side of keeping existing behavior so I wouldn't make the tests more brittle by accident, but I guess we can CHECK for the whole movl (%esp), %eax that returns the return address. What do you think?

test/CodeGen/X86/2004-02-14-InefficientStackPointer.ll

5–6 ↗

(On Diff #87941)

Changed the check to just sub %esp so we don't use a regex.

There's already -march=x86. Do we need to specify the whole triple? I can add -x86-asm-syntax=att if we don't want to rely on it being the default.

[chandlerc]

test/CodeGen/X86/2004-02-13-FrameReturnAddress.ll

10–11 ↗

(On Diff #87941)

Totally right about this needing to check the *label* as its asm at this point. Sorry about that.

Regarding the pattern, the only thing I can think of would be maybe different call frame layouts where there is an offset from %esp.

But you could probably add something like this to handle a displacement:

movl {{.*}}(%esp), %eax

It doesn't really make sense to have a dynamic offset for the return address, but this will handle any constant offset. If it fails for a build bot you can try to make it more clean.

test/CodeGen/X86/2004-02-14-InefficientStackPointer.ll

5–6 ↗

(On Diff #87941)

This is probably fine.

The only concern I have is if there is a difference between ABIs. I would probably add the triple to the IR file itself (we used to not have that facility but we do now) and I often use something like "i686-unknown-unknown" to be fully generic. Anyways, not strictly necessary, can wait and see what the build bots say.

test/CodeGen/X86/2006-01-19-ISelFoldingBug.ll

18 ↗

(On Diff #88078)

Missing an entry label here.

[jgorbe]

• Added explicit target triple to test/CodeGen/X86/2004-02-14-InefficientStackPointer.ll
• Added regex to catch possible constant displacements from %esp in test/CodeGen/X86/2004-02-13-FrameReturnAddress.ll
• Added missing entry label.

[jgorbe]

Some more answers to inline comments. Thanks!

test/CodeGen/X86/2004-02-13-FrameReturnAddress.ll

10–11 ↗

(On Diff #87941)

Added regexp to match constant displacements.

test/CodeGen/X86/2004-02-14-InefficientStackPointer.ll

5–6 ↗

(On Diff #87941)

Thanks for the explanation. Added target triple :)

test/CodeGen/X86/2006-01-19-ISelFoldingBug.ll

18 ↗

(On Diff #88078)

Added, thanks.

[chandlerc]

LGTM with a tiny nit pick. I'll apply the fix and submit momentarily, but mentioning it here just for posterity.

test/CodeGen/X86/2005-01-17-CycleInDAG.ll

18 ↗

(On Diff #88236)

Tiny nit pick: you can often narrow the scope of the regex, and I often find that easier to read. So I would usually write:

; CHECK-NOT: sub{{.*}}GLOBAL

[jgorbe]

test/CodeGen/X86/2005-01-17-CycleInDAG.ll

18 ↗

(On Diff #88236)

Note taken, thanks!

llvm/trunk/test/CodeGen/X86/2003-11-03-GlobalBool.ll

-; RUN: llc < %s -march=x86 | \
-; RUN:   not grep ".byte[[:space:]]*true"
+; RUN: llc < %s -march=x86 | FileCheck %s
 
-@X = global i1 true             ; <i1*> [#uses=0]
+@X = global i1 true
+; CHECK-NOT: .byte true

llvm/trunk/test/CodeGen/X86/2004-02-13-FrameReturnAddress.ll

-; RUN: llc < %s -march=x86 | grep "(%esp"
-; RUN: llc < %s -march=x86 | grep "pushl  %ebp" | count 1
-; RUN: llc < %s -march=x86 | grep "popl %ebp" | count 1
+; RUN: llc < %s -march=x86 | FileCheck %s
 
 declare i8* @llvm.returnaddress(i32)
 
 declare i8* @llvm.frameaddress(i32)
 
 define i8* @test1() {
-        %X = call i8* @llvm.returnaddress( i32 0 )              ; <i8*> [#uses=1]
-        ret i8* %X
+; CHECK-LABEL: test1:
+entry:
+  %X = call i8* @llvm.returnaddress( i32 0 )
+; CHECK: movl {{.*}}(%esp), %eax
 }
 
 define i8* @test2() {
-        %X = call i8* @llvm.frameaddress( i32 0 )               ; <i8*> [#uses=1]
-        ret i8* %X
+; CHECK-LABEL: test2:
+entry:
+  %X = call i8* @llvm.frameaddress( i32 0 )
+; CHECK: pushl %ebp
+; CHECK: popl %ebp
 }
 

llvm/trunk/test/CodeGen/X86/2004-02-14-InefficientStackPointer.ll

-; RUN: llc < %s -march=x86 | grep -i ESP | not grep sub
+; RUN: llc < %s -march=x86 | FileCheck %s
+
+target triple = "i686-unknown-unknown"
 
 define i32 @test(i32 %X) {
-        ret i32 %X
+; CHECK-LABEL: test:
+entry:
+; CHECK-NOT: subl %esp
 }

llvm/trunk/test/CodeGen/X86/2005-01-17-CycleInDAG.ll

 ; This testcase was distilled from 132.ijpeg.  Bsaically we cannot fold the
 ; load into the sub instruction here as it induces a cycle in the dag, which
 ; is invalid code (there is no correct way to order the instruction).  Check
 ; that we do not fold the load into the sub.
 
-; RUN: llc < %s -march=x86 | not grep sub.*GLOBAL
+; RUN: llc < %s -march=x86 | FileCheck %s
 
-@GLOBAL = external global i32           ; <i32*> [#uses=1]
+@GLOBAL = external global i32
 
 define i32 @test(i32* %P1, i32* %P2, i32* %P3) nounwind {
-        %L = load i32, i32* @GLOBAL          ; <i32> [#uses=1]
-        store i32 12, i32* %P2
-        %Y = load i32, i32* %P3              ; <i32> [#uses=1]
-        %Z = sub i32 %Y, %L             ; <i32> [#uses=1]
-        ret i32 %Z
+; CHECK-LABEL: test:
+entry:
+  %L = load i32, i32* @GLOBAL
+  %Y = load i32, i32* %P3
+  %Z = sub i32 %Y, %L
+; CHECK-NOT: {{sub.*GLOBAL}}
 }
 

llvm/trunk/test/CodeGen/X86/2005-02-14-IllegalAssembler.ll

-; RUN: llc < %s -march=x86 | not grep 18446744073709551612
+; RUN: llc < %s -march=x86 | FileCheck %s
 
 @A = external global i32                ; <i32*> [#uses=1]
 @Y = global i32* getelementptr (i32, i32* @A, i32 -1)                ; <i32**> [#uses=0]
+; CHECK-NOT: 18446744073709551612
 

llvm/trunk/test/CodeGen/X86/2006-01-19-ISelFoldingBug.ll

-; RUN: llc < %s -march=x86 -mcpu=generic | \
-; RUN:   grep shld | count 1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -march=x86 -mcpu=generic | FileCheck %s
 ;
 ; Check that the isel does not fold the shld, which already folds a load
 ; and has two uses, into a store.
 
-@A = external global i32                ; <i32*> [#uses=2]
+@A = external global i32
 
 define i32 @test5(i32 %B, i8 %C) {
-        %tmp.1 = load i32, i32* @A           ; <i32> [#uses=1]
-        %shift.upgrd.1 = zext i8 %C to i32              ; <i32> [#uses=1]
-        %tmp.2 = shl i32 %tmp.1, %shift.upgrd.1         ; <i32> [#uses=1]
-        %tmp.3 = sub i8 32, %C          ; <i8> [#uses=1]
-        %shift.upgrd.2 = zext i8 %tmp.3 to i32          ; <i32> [#uses=1]
-        %tmp.4 = lshr i32 %B, %shift.upgrd.2            ; <i32> [#uses=1]
-        %tmp.5 = or i32 %tmp.4, %tmp.2          ; <i32> [#uses=2]
-        store i32 %tmp.5, i32* @A
-        ret i32 %tmp.5
+; CHECK-LABEL: test5:
+; CHECK:       # BB#0:
+; CHECK-NEXT:    movb {{[0-9]+}}(%esp), %cl
+; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %edx
+; CHECK-NEXT:    movl A, %eax
+; CHECK-NEXT:    shldl %cl, %edx, %eax
+; CHECK-NEXT:    movl %eax, A
+; CHECK-NEXT:    retl
+entry:
+  %tmp.1 = load i32, i32* @A
+  %shift.upgrd.1 = zext i8 %C to i32
+  %tmp.2 = shl i32 %tmp.1, %shift.upgrd.1
+  %tmp.3 = sub i8 32, %C
+  %shift.upgrd.2 = zext i8 %tmp.3 to i32
+  %tmp.4 = lshr i32 %B, %shift.upgrd.2
+  %tmp.5 = or i32 %tmp.4, %tmp.2
 }