[PATCH] D29807: FileCheck-ize some tests in test/CodeGen/X86/

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

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!



================
Comment at: test/CodeGen/X86/2003-11-03-GlobalBool.ll:2
+; RUN: llc < %s -march=x86 | FileCheck %s
+; CHECK-NOT: {{.byte[[:space:]]*true}}
 
----------------
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.


================
Comment at: test/CodeGen/X86/2004-02-13-FrameReturnAddress.ll:10-11
         ret i8* %X
+; CHECK-LABEL: test1
+; CHECK: (%esp
 }
----------------
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.


================
Comment at: test/CodeGen/X86/2004-02-14-InefficientStackPointer.ll:5-6
         ret i32 %X
+; CHECK-LABEL: test
+; CHECK-NOT: {{sub.*esp}}
 }
----------------
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.


================
Comment at: test/CodeGen/X86/2006-01-19-ISelFoldingBug.ll:18-20
+; CHECK-LABEL: test5
+; CHECK: shld
+; CHECK-NOT: shld
----------------
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.


https://reviews.llvm.org/D29807