This is an archive of the discontinued LLVM Phabricator instance.

Differential D20905

Only attempt to detect AVG if SSE2 is available
ClosedPublic

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

Details

Reviewers
spatel
eli.friedman
congh
Commits
rG6a482a73d6f0: Only attempt to detect AVG if SSE2 is available
rL271548: Only attempt to detect AVG if SSE2 is available
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

You can remove the 'else':
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

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.

Closed by commit rL271548: Only attempt to detect AVG if SSE2 is available (authored by dim). · Explain WhyJun 2 2016, 10:37 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Target/
X86/
2 lines
test/
CodeGen/
X86/
32 lines

Diff 59418

lib/Target/X86/X86ISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 27,836 Lines • ▼ Show 20 Lines if (!((ScalarVT == MVT::i8 || ScalarVT == MVT::i16) &&
return SDValue(); return SDValue();
// InScalarVT is the intermediate type in AVG pattern and it should be greater // InScalarVT is the intermediate type in AVG pattern and it should be greater
// than the original input type (i8/i16). // than the original input type (i8/i16).
EVT InScalarVT = InVT.getVectorElementType(); EVT InScalarVT = InVT.getVectorElementType();
if (InScalarVT.getSizeInBits() <= ScalarVT.getSizeInBits()) if (InScalarVT.getSizeInBits() <= ScalarVT.getSizeInBits())
return SDValue(); return SDValue();
if (!Subtarget.hasSSE2())
return SDValue();
if (Subtarget.hasAVX512()) { if (Subtarget.hasAVX512()) {
if (VT.getSizeInBits() > 512) if (VT.getSizeInBits() > 512)
return SDValue(); return SDValue();
spatelUnsubmitted
Not Done
ReplyInline Actions

You can remove the 'else':
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

spatel: You can remove the 'else': http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a…
} else if (Subtarget.hasAVX2()) { } else if (Subtarget.hasAVX2()) {
if (VT.getSizeInBits() > 256) if (VT.getSizeInBits() > 256)
return SDValue(); return SDValue();
} else { } else {
if (VT.getSizeInBits() > 128) if (VT.getSizeInBits() > 128)
return SDValue(); return SDValue();
} }
▲ Show 20 Lines • Show All 3,203 Lines • Show Last 20 Lines

test/CodeGen/X86/no-sse2-avg.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; REQUIRES: asserts
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=-sse2 | FileCheck %s
define <16 x i8> @PR27973() {
; CHECK-LABEL: PR27973:
; CHECK: # BB#0:
; CHECK-NEXT: movb $0, 15(%rdi)
; CHECK-NEXT: movb $0, 14(%rdi)
; CHECK-NEXT: movb $0, 13(%rdi)
; CHECK-NEXT: movb $0, 12(%rdi)
; CHECK-NEXT: movb $0, 11(%rdi)
; CHECK-NEXT: movb $0, 10(%rdi)
; CHECK-NEXT: movb $0, 9(%rdi)
; CHECK-NEXT: movb $0, 8(%rdi)
; CHECK-NEXT: movb $0, 7(%rdi)
; CHECK-NEXT: movb $0, 6(%rdi)
; CHECK-NEXT: movb $0, 5(%rdi)
; CHECK-NEXT: movb $0, 4(%rdi)
; CHECK-NEXT: movb $0, 3(%rdi)
; CHECK-NEXT: movb $0, 2(%rdi)
; CHECK-NEXT: movb $0, 1(%rdi)
; CHECK-NEXT: movb $0, (%rdi)
; CHECK-NEXT: movq %rdi, %rax
; CHECK-NEXT: retq
;
%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
}