[X86}: Change fixup-bw-inst.ll to test output with this optimization on and off.

[kbsmith1]

Summary

This expands the unit test for X86FixupBWInsts.cpp by testing for the expected output
both when the optimization is turned on, and turned off.

[kbsmith1]

Sanjay - See if you like this form for testing with fixup-byte-word-insts both on and off. If you prefer this, then I
could make similar changes to the few other tests that are also affected when fixup-byte-word-insts would change
to be turned on by default.

[spatel]

This was one of the options that I suggested in D17173, so I think it's fine to use an additional run on the affected tests.
I'd like to know if Quentin agrees though.

[qcolombet]

Hi,

Sounds good to me.
Thanks Sanjay for pointing thread out and thanks Kevin for your patient with our requests!

You have a typo in your check lines or command line depending on how you what to fix that though.

See my inline comment.

Cheers,
-Quentin

fixup-bw-inst.ll

18

You are actually not checking anything here.

-check-prefix BWON will check prefix that starts with BWON.
If you want CHECK-BWON, you need to use -check-prefix CHECK-BWON.

[kbsmith1]

Fixes problem that Quentin pointed out.

[kbsmith1]

Doh - missed one instance. Now fixed.

[spatel]

LGTM.
A few notes about LLVM testing in general:

1. For short codegen tests like these, we've been encouraging the use of utils/update_llc_test_checks.py . It automates the check-line creation, so it would've avoided the bug that Quentin noticed. Feel free to try it here. :) After the script does its work, you can reduce the checks as you'd like...
2. But we've been burned by loose regression test checking many times ( https://llvm.org/bugs/show_bug.cgi?id=22897 ). So again, having tighter check lines via the script has benefits...
3. But I understand the point you made in D17173 about test maintenance and focused unit tests. It's an engineering trade-off. If you've had to chase down a few miscompiles, I think you lean towards more tests and more checks rather than less. :)

[kbsmith1]

Thanks for the tips Sanjay.

fixup-bw-inst.ll

-; RUN: llc -fixup-byte-word-insts -march=x86-64 < %s | FileCheck %s
+; RUN: llc -fixup-byte-word-insts=1 -march=x86-64 < %s | \
+; RUN: FileCheck -check-prefix CHECK -check-prefix BWON %s
+; RUN: llc -fixup-byte-word-insts=0 -march=x86-64 < %s | \
+; RUN: FileCheck -check-prefix CHECK -check-prefix BWOFF %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.8.0"
 
 %struct.A = type { i8, i8, i8, i8, i8, i8, i8, i8 }
 
 ; This has byte loads interspersed with byte stores, in a single
 ; basic-block loop.  The upper portion should be dead, so the movb loads
 ; should have been changed into movzbl instead.
 ; TODO: The second movb load doesn't get fixed due to register liveness
 ; not being accurate enough.
 ; CHECK-LABEL: foo1
 ; load:
-; CHECK: movzbl
+; CHECK-BWON:  movzbl
+; CHECK-BWOFF: movb
 ; store:
 ; CHECK: movb
 ; load:
 ; CHECK: movb
 ; store:
 ; CHECK: movb
 ; CHECK: ret
 define void @foo1(i32 %count,

 
 ; This has word loads interspersed with word stores.
 ; The upper portion should be dead, so the movw loads should have
 ; been changed into movzwl instead.
 ; TODO: The second movw load doesn't get fixed due to register liveness
 ; not being accurate enough.
 ; CHECK-LABEL: foo2
 ; load:
-; CHECK: movzwl
+; CHECK-BWON:  movzwl
+; CHECK-BWOFF: movw
 ; store:
 ; CHECK: movw
 ; load:
 ; CHECK: movw
 ; store:
 ; CHECK: movw
 ; CHECK: ret
 define void @foo2(i32 %count,
define void @foo3(i8 *%dst, i8 *%src) {
   store i8 %t0, i8 *%dst, align 1
   ret void
 }
 
 ; This test contains nothing but a simple word load and store.  Since
 ; movw and movzwl are the same size, we should always choose to use
 ; movzwl instead.
 ; CHECK-LABEL: foo4:
-; CHECK: movzwl
+; CHECK-BWON:  movzwl
+; CHECK-BWOFF: movw
 ; CHECK: movw
 define void @foo4(i16 *%dst, i16 *%src) {
   %t0 = load i16, i16 *%src, align 2
   store i16 %t0, i16 *%dst, align 2
   ret void
 }