This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kbsmith1 on Feb 18 2016, 2:15 PM.

Details

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.

Diff Detail

Event Timeline

kbsmith1 updated this revision to Diff 48402.Feb 18 2016, 2:15 PM
kbsmith1 retitled this revision from to [X86}: Change fixup-bw-inst.ll to test output with this optimization on and off..
kbsmith1 updated this object.
kbsmith1 added a reviewer: spatel.
kbsmith1 added a subscriber: llvm-commits.

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.

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 edited edge metadata.Feb 18 2016, 4:54 PM

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–19

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 updated this revision to Diff 48433.Feb 18 2016, 5:13 PM
kbsmith1 edited edge metadata.

Fixes problem that Quentin pointed out.

kbsmith1 marked an inline comment as done.Feb 18 2016, 5:14 PM
kbsmith1 updated this revision to Diff 48434.Feb 18 2016, 5:16 PM

Doh - missed one instance. Now fixed.

spatel accepted this revision.Feb 19 2016, 7:55 AM
spatel edited edge metadata.

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. :)
This revision is now accepted and ready to land.Feb 19 2016, 7:55 AM

Thanks for the tips Sanjay.

This revision was automatically updated to reflect the committed changes.