This is an archive of the discontinued LLVM Phabricator instance.

Differential D26449

DAGCombiner: fix combine of trunc and select
ClosedPublic

Authored by AsafBadouh on Nov 9 2016, 6:25 AM.

Details

Reviewers
spatel
zvi
eladcohen
craig.topper
igorb
Commits
rGb573553424cd: DAGCombiner: fix combine of trunc and select
rL286938: DAGCombiner: fix combine of trunc and select
Summary

the pattern catch the following:
// trunc (select c, a, b) -> select c, (trunc a), (trunc b)

in case select has more than one use the exist pattern will add extra select.

i16 t1 = select c, a , b
i16 t2 = srl t1, 3
i8 t3 = trunc i16 t1 to i8
-->
i8 a1 = trunc i16 a to i8
i8 b1 = trunc i16 a to i8
i8 t3 = select c, a1 , b1
i16 t1 = select c, a , b
i16 t2 = srl t1, 3

Diff Detail

Repository
rL LLVM

Event Timeline

AsafBadouh updated this revision to Diff 77341.Nov 9 2016, 6:25 AM
AsafBadouh retitled this revision from to DAGCombiner: fix combine of trunc and select.
AsafBadouh updated this object.
AsafBadouh added reviewers: zvi, eladcohen, igorb, craig.topper.
AsafBadouh set the repository for this revision to rL LLVM.
AsafBadouh added a subscriber: llvm-commits.
AsafBadouh updated this revision to Diff 77345.Nov 9 2016, 6:59 AM
zvi added inline comments.Nov 9 2016, 8:05 AM
test/CodeGen/X86/select.ll
519

Can this test be reduced to a smaller example?
Please add a comment explaining what this test checks.

AsafBadouh updated this revision to Diff 77475.Nov 10 2016, 6:06 AM
AsafBadouh added a reviewer: spatel.

+ Sanjay
fixes pr29002
update the test case using the example in pr29002

https://llvm.org/bugs/show_bug.cgi?id=29002

andreadb added a subscriber: andreadb.Nov 10 2016, 6:11 AM
spatel edited edge metadata.Nov 10 2016, 6:33 AM

Thanks for tracking this down!

  1. Please use the regression test from:

https://llvm.org/bugs/show_bug.cgi?id=29002#c1

I think that's the minimal case. If you want to include the original test in this patch, I suppose that's ok, but I don't think it actually adds value. Zvi's example in:
https://llvm.org/bugs/show_bug.cgi?id=29002#c2
probably is interesting as it shows a different output?

  1. Please use utils/update_llc_test_checks.py to auto-generate complete checks (and then remove any lines that you think are irrelevant).
AsafBadouh updated this revision to Diff 77497.Nov 10 2016, 8:47 AM
AsafBadouh edited edge metadata.
  1. update the regression tests.
  2. used the utils/update_llc_test_checks.py to auto-generate complete checks.
AsafBadouh marked an inline comment as done.Nov 10 2016, 8:48 AM
zvi edited edge metadata.Nov 10 2016, 9:53 AM

Thanks, Asaf. LGTM. Please make sure you are running the tests on all targets.

Closed by commit rL286938: DAGCombiner: fix combine of trunc and select (authored by abadouh). · Explain WhyNov 15 2016, 12:05 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
CodeGen/
SelectionDAG/
DAGCombiner.cpp
DAGCombiner.cpp (revision 286363)
2 lines
test/
CodeGen/
X86/
select.ll
select.ll (revision 286363)
22 lines

Diff 77475

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 7,299 Lines • ▼ Show 20 Lines if (isa<ConstantSDNode>(EltNo) && isTypeLegal(NVT)) {
SDLoc DL(N); SDLoc DL(N);
return DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, TrTy, return DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, TrTy,
DAG.getBitcast(NVT, N0.getOperand(0)), DAG.getBitcast(NVT, N0.getOperand(0)),
DAG.getConstant(Index, DL, IndexTy)); DAG.getConstant(Index, DL, IndexTy));
} }
} }
// trunc (select c, a, b) -> select c, (trunc a), (trunc b) // trunc (select c, a, b) -> select c, (trunc a), (trunc b)
if (N0.getOpcode() == ISD::SELECT) { if (N0.getOpcode() == ISD::SELECT && N0.hasOneUse()) {
EVT SrcVT = N0.getValueType(); EVT SrcVT = N0.getValueType();
if ((!LegalOperations || TLI.isOperationLegal(ISD::SELECT, SrcVT)) && if ((!LegalOperations || TLI.isOperationLegal(ISD::SELECT, SrcVT)) &&
TLI.isTruncateFree(SrcVT, VT)) { TLI.isTruncateFree(SrcVT, VT)) {
SDLoc SL(N0); SDLoc SL(N0);
SDValue Cond = N0.getOperand(0); SDValue Cond = N0.getOperand(0);
SDValue TruncOp0 = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(1)); SDValue TruncOp0 = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(1));
SDValue TruncOp1 = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(2)); SDValue TruncOp1 = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(2));
return DAG.getNode(ISD::SELECT, SDLoc(N), VT, Cond, TruncOp0, TruncOp1); return DAG.getNode(ISD::SELECT, SDLoc(N), VT, Cond, TruncOp0, TruncOp1);
▲ Show 20 Lines • Show All 8,148 Lines • Show Last 20 Lines

test/CodeGen/X86/select.ll

Show First 20 Lines • Show All 509 Lines • ▼ Show 20 Lines
; CHECK-NEXT: shll %cl, %edi ; CHECK-NEXT: shll %cl, %edi
; CHECK-NEXT: movl %edi, %eax ; CHECK-NEXT: movl %edi, %eax
; CHECK-NEXT: retq ; CHECK-NEXT: retq
%tmp1 = select i1 %cc, i32 3, i32 2 %tmp1 = select i1 %cc, i32 3, i32 2
%tmp2 = shl i32 %a, %tmp1 %tmp2 = shl i32 %a, %tmp1
ret i32 %tmp2 ret i32 %tmp2
} }
define void @clamp(float* %src, i16* %dst) {
; verify that 2 select will not became 3 cmovs
zviUnsubmitted
Done
ReplyInline Actions

Can this test be reduced to a smaller example?
Please add a comment explaining what this test checks.

zvi: Can this test be reduced to a smaller example? Please add a comment explaining what this test…
; fixes pr29002
entry:
; CHECK-LABEL: clamp:
; CHECK: ## BB#0:
; CHECK: cmovlel
; CHECK: cmovgew
; CHECK-NOT: cmov
; CHECK: retq
%ld = load float, float* %src, align 4
%mul = fmul float %ld, 3.276700e+04
%conv = fptosi float %mul to i32
%cmp = icmp sgt i32 %conv, 32767
%sel1 = select i1 %cmp, i32 32767, i32 %conv
%cmp2 = icmp slt i32 %sel1, -32768
%sel2 = select i1 %cmp2, i32 -32768, i32 %sel1
%conv6 = trunc i32 %sel2 to i16
store i16 %conv6, i16* %dst, align 2
ret void
}
define void @test19() { define void @test19() {
; This is a massive reduction of an llvm-stress test case that generates ; This is a massive reduction of an llvm-stress test case that generates
; interesting chains feeding setcc and eventually a f32 select operation. This ; interesting chains feeding setcc and eventually a f32 select operation. This
; is intended to exercise the SELECT formation in the DAG combine simplifying ; is intended to exercise the SELECT formation in the DAG combine simplifying
; a simplified select_cc node. If it it regresses and is no longer triggering ; a simplified select_cc node. If it it regresses and is no longer triggering
; that code path, it can be deleted. ; that code path, it can be deleted.
; ;
; CHECK-LABEL: test19: ; CHECK-LABEL: test19:
▲ Show 20 Lines • Show All 42 Lines • Show Last 20 Lines