[llvm] r208640 - [DAGCombiner] Split up an indexed load if only the base pointer value is live
Adam Nemet
anemet at apple.com
Thu Jun 12 22:24:53 PDT 2014
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’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
More information about the llvm-commits
mailing list