Page MenuHomePhabricator

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

Authored by jgorbe on Feb 9 2017, 7:08 PM.

Diff Detail


Event Timeline

jgorbe created this revision.Feb 9 2017, 7:08 PM
jgorbe added a subscriber: llvm-commits.

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

chandlerc edited edge metadata.Feb 9 2017, 7:11 PM

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 added a comment.Feb 9 2017, 7:16 PM

Noted, thanks!

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!

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.

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.

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.

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/ to generate precise and clean checks for them.

jgorbe updated this revision to Diff 88078.Feb 10 2017, 7:19 PM
jgorbe marked an inline comment as done.

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/
  • Removed a couple of unnecessary regular expressions
  • Changed some test CHECKs to be more accurate than the existing grep search strings.

Some responses to @chandlerc 's previous comments.

2 ↗(On Diff #87941)

Done, thanks!

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?

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 added inline comments.Feb 10 2017, 7:29 PM
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.

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.

18 ↗(On Diff #88078)

Missing an entry label here.

jgorbe updated this revision to Diff 88236.Feb 13 2017, 11:59 AM
jgorbe marked 3 inline comments as done.
  • 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.

Some more answers to inline comments. Thanks!

10–11 ↗(On Diff #87941)

Added regexp to match constant displacements.

5–6 ↗(On Diff #87941)

Thanks for the explanation. Added target triple :)

18 ↗(On Diff #88078)

Added, thanks.

chandlerc accepted this revision.Feb 16 2017, 4:17 PM

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

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
This revision is now accepted and ready to land.Feb 16 2017, 4:17 PM
jgorbe added inline comments.Feb 16 2017, 4:19 PM
18 ↗(On Diff #88236)

Note taken, thanks!

This revision was automatically updated to reflect the committed changes.