[llvm] r243500 - ignore duplicate divisor uses when transforming into reciprocal multiplies (PR24141)

[Sanjay Patel]

Hi Hans -

Yes, that looks like a sufficient fix for the bug. It won't have the
optimization provided by r243498:
http://llvm.org/viewvc/llvm-project?view=revision&revision=243498

...but the SetVector should be all that's needed to avoid the duplicates in
the list that caused the crash.

Implementation/design question: does anyone know why we have duplicate
users in the uses() list in the first place? Ie, why do we implement uses
as a linked list rather than a set?

On Wed, Jul 29, 2015 at 9:41 AM, Hans Wennborg <hans at chromium.org> wrote:

> Thanks everyone! Merged in
> http://llvm.org/viewvc/llvm-project?rev=243524&view=rev
>
> Sanjay: there were merge conflicts due to r243293 not being in the
> branch. Can you double check that I got it right?
>
> On Tue, Jul 28, 2015 at 10:03 PM, Owen Anderson <resistor at mac.com> wrote:
> > OK to merge.
> >
> > —Owen
> >
> >> On Jul 28, 2015, at 6:00 PM, Hans Wennborg <hans at chromium.org> wrote:
> >>
> >> We should probably merge this to 3.7.
> >>
> >> Owen, you're the owner here. OK to merge?
> >>
> >> On Tue, Jul 28, 2015 at 4:28 PM, Sanjay Patel <spatel at rotateright.com>
> wrote:
> >>> Author: spatel
> >>> Date: Tue Jul 28 18:28:22 2015
> >>> New Revision: 243500
> >>>
> >>> URL: http://llvm.org/viewvc/llvm-project?rev=243500&view=rev
> >>> Log:
> >>> ignore duplicate divisor uses when transforming into reciprocal
> multiplies (PR24141)
> >>>
> >>> PR24141: https://llvm.org/bugs/show_bug.cgi?id=24141
> >>> contains a test case where we have duplicate entries in a node's
> uses() list.
> >>>
> >>> After r241826, we use CombineTo() to delete dead nodes when combining
> the uses into
> >>> reciprocal multiplies, but this fails if we encounter the just-deleted
> node again in
> >>> the list.
> >>>
> >>> The solution in this patch is to not add duplicate entries to the list
> of users that
> >>> we will subsequently iterate over. For the test case, this avoids
> triggering the
> >>> combine divisors logic entirely because there really is only one user
> of the divisor.
> >>>
> >>> Differential Revision: http://reviews.llvm.org/D11345
> >>>
> >>>
> >>> Modified:
> >>>    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >>>    llvm/trunk/test/CodeGen/X86/fdiv-combine.ll
> >>>
> >>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >>> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=243500&r1=243499&r2=243500&view=diff
> >>>
> ==============================================================================
> >>> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> >>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Tue Jul 28
> 18:28:22 2015
> >>> @@ -8261,11 +8261,11 @@ SDValue DAGCombiner::combineRepeatedFPDi
> >>>     return SDValue();
> >>>
> >>>   // Find all FDIV users of the same divisor.
> >>> -  SmallVector<SDNode *, 4> Users;
> >>> -  for (auto *U : N1->uses()) {
> >>> +  // Use a set because duplicates may be present in the user list.
> >>> +  SetVector<SDNode *> Users;
> >>> +  for (auto *U : N1->uses())
> >>>     if (U->getOpcode() == ISD::FDIV && U->getOperand(1) == N1)
> >>> -      Users.push_back(U);
> >>> -  }
> >>> +      Users.insert(U);
> >>>
> >>>   // Now that we have the actual number of divisor uses, make sure it
> meets
> >>>   // the minimum threshold specified by the target.
> >>>
> >>> Modified: llvm/trunk/test/CodeGen/X86/fdiv-combine.ll
> >>> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fdiv-combine.ll?rev=243500&r1=243499&r2=243500&view=diff
> >>>
> ==============================================================================
> >>> --- llvm/trunk/test/CodeGen/X86/fdiv-combine.ll (original)
> >>> +++ llvm/trunk/test/CodeGen/X86/fdiv-combine.ll Tue Jul 28 18:28:22
> 2015
> >>> @@ -44,5 +44,24 @@ define double @div3_arcp(double %x, doub
> >>>   ret double %ret
> >>> }
> >>>
> >>> +define void @PR24141() #0 {
> >>> +; CHECK-LABEL: PR24141:
> >>> +; CHECK:       callq
> >>> +; CHECK-NEXT:  divsd
> >>> +; CHECK-NEXT:  jmp
> >>> +entry:
> >>> +  br label %while.body
> >>> +
> >>> +while.body:
> >>> +  %x.0 = phi double [ undef, %entry ], [ %div, %while.body ]
> >>> +  %call = call { double, double } @g(double %x.0)
> >>> +  %xv0 = extractvalue { double, double } %call, 0
> >>> +  %xv1 = extractvalue { double, double } %call, 1
> >>> +  %div = fdiv double %xv0, %xv1
> >>> +  br label %while.body
> >>> +}
> >>> +
> >>> +declare { double, double } @g(double)
> >>> +
> >>> ; FIXME: If the backend understands 'arcp', then this attribute is
> unnecessary.
> >>> attributes #0 = { "unsafe-fp-math"="true" }
> >>>
> >>>
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150729/8f015059/attachment.html>