[llvm] r208640 - [DAGCombiner] Split up an indexed load if only the base pointer value is live
Adam Nemet
anemet at apple.com
Tue Sep 2 10:40:51 PDT 2014
On Sep 1, 2014, at 11:42 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Adam Nemet" <anemet at apple.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: llvm-commits at cs.uiuc.edu
>> Sent: Friday, June 13, 2014 12:24:53 AM
>> Subject: Re: [llvm] r208640 - [DAGCombiner] Split up an indexed load if only the base pointer value is live
>>
>>
>> On Jun 12, 2014, at 5:25 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>
>>> ----- Original Message -----
>>>> From: "Adam Nemet" <anemet at apple.com>
>>>> To: "Hal Finkel" <hfinkel at anl.gov>
>>>> Cc: llvm-commits at cs.uiuc.edu
>>>> Sent: Thursday, June 12, 2014 6:11:19 PM
>>>> Subject: Re: [llvm] r208640 - [DAGCombiner] Split up an indexed
>>>> load if only the base pointer value is live
>>>>
>>>> Hal,
>>>>
>>>> Did you guys make any progress on this?
>>>
>>> Not yet, but it is next on my list (I'm working on PR19991 right
>>> now).
>>
>> Great, thanks!
>
> I'm really sorry that it took me so long to actually get to this. Reapplied with modifications in r216898.
No problem, thanks very much for the fix!
Adam
> Thanks again,
> Hal
>
>>
>>>> I’d like to see this
>>>> committed back possibly with some fixes.
>>>
>>> Me too :-)
>>>
>>>>
>>>> Let me know if I can help. Is there a PR tracking the issue?
>>>
>>> Will do; I'm not sure if anyone filed a PR or not.
>>
>> It's PR20024 now.
>>
>> Adam
>>
>>>
>>> -Hal
>>>
>>>>
>>>> Thanks,
>>>> Adam
>>>>
>>>> On May 28, 2014, at 9:57 AM, Adam Nemet <anemet at apple.com> wrote:
>>>>
>>>>> Hi Hal,
>>>>>
>>>>> Sure, sorry about that. Let me know if I can help in any way.
>>>>>
>>>>> You may want to try disable the transformation in the presence of
>>>>> TargetConstants. This is what cause the issue in PR19796.
>>>>> Something like:
>>>>>
>>>>> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>>> b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>>> index 2d2fd53..ed3c581 100644
>>>>> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>>> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>>> @@ -7899,7 +7899,8 @@ SDValue DAGCombiner::visitLOAD(SDNode *N) {
>>>>> if (!N->hasAnyUseOfValue(0)) {
>>>>> SDValue Undef = DAG.getUNDEF(N->getValueType(0));
>>>>> SDValue Index;
>>>>> - if (N->hasAnyUseOfValue(1)) {
>>>>> + if (N->hasAnyUseOfValue(1) &&
>>>>> + N->getOperand(1)->getOpcode() !=
>>>>> ISD::TargetConstant)
>>>>> {
>>>>> Index = SplitIndexingFromLoad(LD);
>>>>> // Try to fold the base pointer arithmetic into
>>>>> subsequent loads and
>>>>> // stores.
>>>>>
>>>>> Adam
>>>>>
>>>>> On May 28, 2014, at 8:45 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>>>>>
>>>>>> Adam,
>>>>>>
>>>>>> Unfortunately, this caused a massive self-hosting failure on
>>>>>> ppc64/Linux. I've reverted it (in r209747) while we investigate.
>>>>>> Given that, with this change, nearly all regression tests
>>>>>> segfault, I hope it will be relatively easy to diagnose.
>>>>>>
>>>>>> -Hal
>>>>>>
>>>>>> ----- Original Message -----
>>>>>>> From: "Adam Nemet" <anemet at apple.com>
>>>>>>> To: llvm-commits at cs.uiuc.edu
>>>>>>> Sent: Monday, May 12, 2014 6:00:03 PM
>>>>>>> Subject: [llvm] r208640 - [DAGCombiner] Split up an indexed
>>>>>>> load
>>>>>>> if only the base pointer value is live
>>>>>>>
>>>>>>> Author: anemet
>>>>>>> Date: Mon May 12 18:00:03 2014
>>>>>>> New Revision: 208640
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=208640&view=rev
>>>>>>> Log:
>>>>>>> [DAGCombiner] Split up an indexed load if only the base pointer
>>>>>>> value
>>>>>>> is live
>>>>>>>
>>>>>>> Right now the load may not get DCE'd because of the side-effect
>>>>>>> of
>>>>>>> updating
>>>>>>> the base pointer.
>>>>>>>
>>>>>>> This can happen if we lower a read-modify-write of an illegal
>>>>>>> larger
>>>>>>> type
>>>>>>> (e.g. i48) such that the modification only affects one of the
>>>>>>> subparts (the
>>>>>>> lower i32 part but not the higher i16 part). See the testcase.
>>>>>>>
>>>>>>> In order to spot the dead load we need to revisit it when
>>>>>>> SimplifyDemandedBits
>>>>>>> decided that the value of the load is masked off. This is the
>>>>>>> CommitTargetLoweringOpt piece.
>>>>>>>
>>>>>>> I checked compile time with ARM64 by sending SPEC bitcode files
>>>>>>> through llc.
>>>>>>> No measurable change.
>>>>>>>
>>>>>>> Fixes <rdar://problem/16031651>
>>>>>>>
>>>>>>> Added:
>>>>>>> llvm/trunk/test/CodeGen/ARM64/dagcombiner-dead-indexed-load.ll
>>>>>>> Modified:
>>>>>>> llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>>>>>
>>>>>>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=208640&r1=208639&r2=208640&view=diff
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>>>>> (original)
>>>>>>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Mon May
>>>>>>> 12
>>>>>>> 18:00:03 2014
>>>>>>> @@ -167,6 +167,7 @@ namespace {
>>>>>>>
>>>>>>> bool CombineToPreIndexedLoadStore(SDNode *N);
>>>>>>> bool CombineToPostIndexedLoadStore(SDNode *N);
>>>>>>> + SDValue SplitIndexingFromLoad(LoadSDNode *LD);
>>>>>>> bool SliceUpLoad(SDNode *N);
>>>>>>>
>>>>>>> void ReplaceLoadWithPromotedLoad(SDNode *Load, SDNode
>>>>>>> *ExtLoad);
>>>>>>> @@ -761,10 +762,14 @@ CommitTargetLoweringOpt(const TargetLowe
>>>>>>>
>>>>>>> // If the operands of this node are only used by the node,
>>>>>>> they
>>>>>>> will now
>>>>>>> // be dead. Make sure to visit them first to delete dead
>>>>>>> nodes
>>>>>>> early.
>>>>>>> - for (unsigned i = 0, e =
>>>>>>> TLO.Old.getNode()->getNumOperands(); i
>>>>>>> != e; ++i)
>>>>>>> - if
>>>>>>> (TLO.Old.getNode()->getOperand(i).getNode()->hasOneUse())
>>>>>>> -
>>>>>>> AddToWorkList(TLO.Old.getNode()->getOperand(i).getNode());
>>>>>>> -
>>>>>>> + for (unsigned i = 0, e =
>>>>>>> TLO.Old.getNode()->getNumOperands(); i
>>>>>>> != e; ++i) {
>>>>>>> + SDNode *Op = TLO.Old.getNode()->getOperand(i).getNode();
>>>>>>> + // For an operand generating multiple values, one of the
>>>>>>> values may
>>>>>>> + // become dead allowing further simplification (e.g.
>>>>>>> split
>>>>>>> index
>>>>>>> + // arithmetic from an indexed load).
>>>>>>> + if (Op->hasOneUse() || Op->getNumValues() > 1)
>>>>>>> + AddToWorkList(Op);
>>>>>>> + }
>>>>>>> DAG.DeleteNode(TLO.Old.getNode());
>>>>>>> }
>>>>>>> }
>>>>>>> @@ -7844,6 +7849,17 @@ bool DAGCombiner::CombineToPostIndexedLo
>>>>>>> return false;
>>>>>>> }
>>>>>>>
>>>>>>> +/// \brief Return the base-pointer arithmetic from an indexed
>>>>>>> \p
>>>>>>> LD.
>>>>>>> +SDValue DAGCombiner::SplitIndexingFromLoad(LoadSDNode *LD) {
>>>>>>> + ISD::MemIndexedMode AM = LD->getAddressingMode();
>>>>>>> + assert(AM != ISD::UNINDEXED);
>>>>>>> + SDValue BP = LD->getOperand(1);
>>>>>>> + SDValue Inc = LD->getOperand(2);
>>>>>>> + unsigned Opc =
>>>>>>> + (AM == ISD::PRE_INC || AM == ISD::POST_INC ? ISD::ADD :
>>>>>>> ISD::SUB);
>>>>>>> + return DAG.getNode(Opc, SDLoc(LD), BP.getSimpleValueType(),
>>>>>>> BP,
>>>>>>> Inc);
>>>>>>> +}
>>>>>>> +
>>>>>>> SDValue DAGCombiner::visitLOAD(SDNode *N) {
>>>>>>> LoadSDNode *LD = cast<LoadSDNode>(N);
>>>>>>> SDValue Chain = LD->getChain();
>>>>>>> @@ -7880,8 +7896,16 @@ SDValue DAGCombiner::visitLOAD(SDNode *N
>>>>>>> } else {
>>>>>>> // Indexed loads.
>>>>>>> assert(N->getValueType(2) == MVT::Other && "Malformed
>>>>>>> indexed
>>>>>>> loads?");
>>>>>>> - if (!N->hasAnyUseOfValue(0) && !N->hasAnyUseOfValue(1))
>>>>>>> {
>>>>>>> + if (!N->hasAnyUseOfValue(0)) {
>>>>>>> SDValue Undef = DAG.getUNDEF(N->getValueType(0));
>>>>>>> + SDValue Index;
>>>>>>> + if (N->hasAnyUseOfValue(1)) {
>>>>>>> + Index = SplitIndexingFromLoad(LD);
>>>>>>> + // Try to fold the base pointer arithmetic into
>>>>>>> subsequent
>>>>>>> loads and
>>>>>>> + // stores.
>>>>>>> + AddUsersToWorkList(N);
>>>>>>> + } else
>>>>>>> + Index = DAG.getUNDEF(N->getValueType(1));
>>>>>>> DEBUG(dbgs() << "\nReplacing.7 ";
>>>>>>> N->dump(&DAG);
>>>>>>> dbgs() << "\nWith: ";
>>>>>>> @@ -7889,8 +7913,7 @@ SDValue DAGCombiner::visitLOAD(SDNode *N
>>>>>>> dbgs() << " and 2 other values\n");
>>>>>>> WorkListRemover DeadNodes(*this);
>>>>>>> DAG.ReplaceAllUsesOfValueWith(SDValue(N, 0), Undef);
>>>>>>> - DAG.ReplaceAllUsesOfValueWith(SDValue(N, 1),
>>>>>>> -
>>>>>>> DAG.getUNDEF(N->getValueType(1)));
>>>>>>> + DAG.ReplaceAllUsesOfValueWith(SDValue(N, 1), Index);
>>>>>>> DAG.ReplaceAllUsesOfValueWith(SDValue(N, 2), Chain);
>>>>>>> removeFromWorkList(N);
>>>>>>> DAG.DeleteNode(N);
>>>>>>>
>>>>>>> Added:
>>>>>>> llvm/trunk/test/CodeGen/ARM64/dagcombiner-dead-indexed-load.ll
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM64/dagcombiner-dead-indexed-load.ll?rev=208640&view=auto
>>>>>>> ==============================================================================
>>>>>>> ---
>>>>>>> llvm/trunk/test/CodeGen/ARM64/dagcombiner-dead-indexed-load.ll
>>>>>>> (added)
>>>>>>> +++
>>>>>>> llvm/trunk/test/CodeGen/ARM64/dagcombiner-dead-indexed-load.ll
>>>>>>> Mon May 12 18:00:03 2014
>>>>>>> @@ -0,0 +1,29 @@
>>>>>>> +; RUN: llc -mcpu=cyclone < %s | FileCheck %s
>>>>>>> +
>>>>>>> +target datalayout = "e-i64:64-n32:64-S128"
>>>>>>> +target triple = "arm64-apple-ios"
>>>>>>> +
>>>>>>> +%"struct.SU" = type { i32, %"struct.SU"*, i32*, i32, i32,
>>>>>>> %"struct.BO", i32, [5 x i8] }
>>>>>>> +%"struct.BO" = type { %"struct.RE" }
>>>>>>> +
>>>>>>> +%"struct.RE" = type { i32, i32, i32, i32 }
>>>>>>> +
>>>>>>> +; This is a read-modify-write of some bifields combined into
>>>>>>> an
>>>>>>> i48.
>>>>>>> It gets
>>>>>>> +; legalized into i32 and i16 accesses. Only a single store of
>>>>>>> zero
>>>>>>> to the low
>>>>>>> +; i32 part should be live.
>>>>>>> +
>>>>>>> +; CHECK-LABEL: test:
>>>>>>> +; CHECK-NOT: ldr
>>>>>>> +; CHECK: str wzr
>>>>>>> +; CHECK-NOT: str
>>>>>>> +define void @test(%"struct.SU"* nocapture %su) {
>>>>>>> +entry:
>>>>>>> + %r1 = getelementptr inbounds %"struct.SU"* %su, i64 1, i32 5
>>>>>>> + %r2 = bitcast %"struct.BO"* %r1 to i48*
>>>>>>> + %r3 = load i48* %r2, align 8
>>>>>>> + %r4 = and i48 %r3, -4294967296
>>>>>>> + %r5 = or i48 0, %r4
>>>>>>> + store i48 %r5, i48* %r2, align 8
>>>>>>> +
>>>>>>> + ret void
>>>>>>> +}
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Hal Finkel
>>>>>> Assistant Computational Scientist
>>>>>> Leadership Computing Facility
>>>>>> Argonne National Laboratory
>>>>>
>>>>
>>>>
>>>
>>> --
>>> Hal Finkel
>>> Assistant Computational Scientist
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/da803391/attachment.html>
More information about the llvm-commits
mailing list