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

Wed Jul 29 09:53:02 PDT 2015

Users are a linked list for numerous reasons, some historical, some still
present. We need to be able to walk the list, and we need control over the
order as LLVM is not (at this time) good about producing deterministic
output in the face of changing order of use lists.

We also, quite fundamentally, do need to track each individual use of a
value. They are the operands of other instructions, and can be changed
independently. We would need something more akin to a reference count than
merely participation in a set.

On Wed, Jul 29, 2015 at 9:11 AM Sanjay Patel <spatel at rotateright.com> wrote:

> 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
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Fview-3Drevision-26revision-3D243498&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=w4_cCnPV5XrJCzrR7v1k_Ck8Ot0z43fRtx1-9XDKQ_w&s=1jyoWko4rqeXrnzBoyMYPzCoOK4I7cdGlrUZeFCPNKg&e=>
>
> ...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
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D243524-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=w4_cCnPV5XrJCzrR7v1k_Ck8Ot0z43fRtx1-9XDKQ_w&s=o-jwp3RlS9xnxiecWhNBBO_Ff7IbW-ZE7cx1l-2F0u4&e=>
>>
>> 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
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D243500-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=w4_cCnPV5XrJCzrR7v1k_Ck8Ot0z43fRtx1-9XDKQ_w&s=5gOxU60u9Nf24K44SCBoePxYmqb-3v8a-EByYcQzifs&e=>
>> >>> Log:
>> >>> ignore duplicate divisor uses when transforming into reciprocal
>> multiplies (PR24141)
>> >>>
>> >>> PR24141: https://llvm.org/bugs/show_bug.cgi?id=24141
>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D24141&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=w4_cCnPV5XrJCzrR7v1k_Ck8Ot0z43fRtx1-9XDKQ_w&s=QWgEmDvopM0TF-U7kusgF8y_3JQ7st2WVivRauPM9oE&e=>
>> >>> 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
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11345&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=w4_cCnPV5XrJCzrR7v1k_Ck8Ot0z43fRtx1-9XDKQ_w&s=T8iJaCEkQmEXAWk7EbtjwSQRSJ8zoXuOxtokyniuDZE&e=>
>> >>>
>> >>>
>> >>> 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
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_CodeGen_SelectionDAG_DAGCombiner.cpp-3Frev-3D243500-26r1-3D243499-26r2-3D243500-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=w4_cCnPV5XrJCzrR7v1k_Ck8Ot0z43fRtx1-9XDKQ_w&s=lxoXiM-esaFU1LBG_aL5m5pdMmKj8NDpCtehhDn6HWY&e=>
>> >>>
>> ==============================================================================
>> >>> --- 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
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_CodeGen_X86_fdiv-2Dcombine.ll-3Frev-3D243500-26r1-3D243499-26r2-3D243500-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=w4_cCnPV5XrJCzrR7v1k_Ck8Ot0z43fRtx1-9XDKQ_w&s=qWPNdNr5F_-VAtwK8ycIXRlK3hNjw17p8ohiwLnjnwY&e=>
>> >>>
>> ==============================================================================
>> >>> --- 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
>> >
>>
>
> _______________________________________________
> 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/8b4f288a/attachment.html>