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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


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


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


================
Comment at: test/CodeGen/X86/2006-01-19-ISelFoldingBug.ll:18
+; CHECK-NEXT:    retl
+  %tmp.1 = load i32, i32* @A
+  %shift.upgrd.1 = zext i8 %C to i32
----------------
Missing an entry label here.


https://reviews.llvm.org/D29807