Page MenuHomePhabricator

[InstCombine] Restrict transforming shared selects using SimplifySelectsFeedingBinaryOp when we cannot simplify the binary op.
AbandonedPublic

Authored by bmakam on Sep 26 2017, 12:03 PM.

Details

Summary

We will still keep the original select if there are multiple uses thus increasing
the number of instructions, so enable this transform for shared selects only when
we can fold the binary op with a select.

Diff Detail

Event Timeline

bmakam created this revision.Sep 26 2017, 12:03 PM
mcrosier edited edge metadata.Sep 26 2017, 12:18 PM

I think we're on the right track, but what about this test?

declare void @use_float(double)
define float @test3(i1 zeroext %arg) #0 {
  %tmp = select i1 %arg, float 5.000000e+00, float 6.000000e+00
  %tmp1 = select i1 %arg, float 1.000000e+00, float 9.000000e+00
  %tmp2 = fmul float %tmp, %tmp1
  call void @use_double(double %tmp)
  ret float %tmp2
}

I think we still want to transform this case (because we'll replace the fmul with a select) even though the first select has multiple uses.

mcrosier added inline comments.Sep 26 2017, 1:01 PM
lib/Transforms/InstCombine/InstructionCombining.cpp
740

I wonder if something like this would work:

bool SelectsHaveOneUse = LHS->hasOneUse() && RHS->hasOneUse();
if (V1 && V2)
  SI = Builder.CreateSelect(A, V2, V1);
else if (V2 && SelectsHaveOneUse)
  SI = Builder.CreateSelect(A, V2, Builder.CreateBinOp(Opcode, C, E));
else if (V1 && SelectsHaveOneUse)
  SI = Builder.CreateSelect(A, Builder.CreateBinOp(Opcode, B, D), V1);
bmakam updated this revision to Diff 116701.Sep 26 2017, 1:24 PM
bmakam retitled this revision from [InstCombine] Predicate SimplifySelectsFeedingBinaryOp with m_OneUse to reduce code. to [InstCombine] Restrict transforming shared selects using SimplifySelectsFeedingBinaryOp when we cannot simplify the binary op..
bmakam edited the summary of this revision. (Show Details)

Address Chad's testcase.

LGTM, but I'll defer to @majnemer and company for the final approval.

mcberg2017 added inline comments.Sep 26 2017, 6:07 PM
lib/Transforms/InstCombine/InstructionCombining.cpp
740

An earlier review was opened here: https://reviews.llvm.org/D38263, from which the snippet above originates. Just to let folks know that are two reviews open on this topic.

mcrosier added inline comments.Sep 27 2017, 6:12 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
740

Somehow I did a drive by comment on D38263 without realizing the two fixes were the same. I'd probably just go ahead and abandon this revision, Balaram.

bmakam abandoned this revision.Sep 27 2017, 7:29 AM

abandon this revision since it is a duplicate of D38263