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

[Jorge Gorbe via Phabricator via llvm-commits]

jgorbe added a comment.

Some more answers to inline comments. Thanks!



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


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


================
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
----------------
chandlerc wrote:
> Missing an entry label here.
Added, thanks.


https://reviews.llvm.org/D29807