This is an archive of the discontinued LLVM Phabricator instance.

Only attempt to detect AVG if SSE2 is available
ClosedPublic

Authored by dim on Jun 2 2016, 5:05 AM.

Details

Summary

In PR29973 I reported an assertion failure when a certain loop was
optimized, for a target without SSE2 support. It turned out this was
because of the AVG pattern detection introduced in rL253952.

Prevent the assertion failure by bailing out early in
detectAVGPattern(), if the target does not support SSE2.

Diff Detail

Event Timeline

dim updated this revision to Diff 59365.Jun 2 2016, 5:05 AM
dim retitled this revision from to Only attempt to detect AVG if SSE2 is available.
dim updated this object.
dim added reviewers: congh, eli.friedman, spatel.
dim added a subscriber: llvm-commits.
dim updated this revision to Diff 59368.Jun 2 2016, 5:47 AM

Add simplified test case from PR29973.

spatel edited edge metadata.Jun 2 2016, 6:12 AM

Thanks for working on the patch, Dimitry.

The test case can be simplified further to at least this:

define <16 x i8> @fn1() {
  %t0 = zext <16 x i8> zeroinitializer to <16 x i32>
  %t1 = add nuw nsw <16 x i32> %t0, <i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1>
  %t2 = lshr <16 x i32> %t1, <i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1, i32 1>
  %t3 = trunc <16 x i32> %t2 to <16 x i8>
  ret <16 x i8> %t3
}

You can use "-mtriple=x86_64-unknown-unknown -mattr=-sse2" in the RUN line and remove all of the attributes/metadata. Please use FileCheck in the RUN line to verify correctness of the output rather than the absence of an assert. You can then use:
$ utils/update_test_checks.py --tool=../../../../build/bin/llc pr27973.ll
to auto-generate the CHECK lines (see other x86 regression tests for examples of what this looks like).

lib/Target/X86/X86ISelLowering.cpp
27849
dim updated this revision to Diff 59378.Jun 2 2016, 7:40 AM
dim edited edge metadata.
  • Removed else after return
  • More test case minimization, and added FileCheck
emaste added a subscriber: emaste.Jun 2 2016, 7:57 AM
spatel accepted this revision.Jun 2 2016, 8:06 AM
spatel edited edge metadata.

LGTM. Thanks!

I was told sometime a while back that we should prefer to give regression test files a meaningful name rather than 'pr#'. Then, you make the function name be '@PR#' rather than '@fn1' or add a comment line to explain further. In this case, a better test file name might be something like 'no-sse2-avg.ll'?

This revision is now accepted and ready to land.Jun 2 2016, 8:06 AM
dim updated this revision to Diff 59418.Jun 2 2016, 10:30 AM
dim edited edge metadata.

Rename test case to no-sse2-avg.ll, and use the PR identifier as the
function name.

This revision was automatically updated to reflect the committed changes.