[llvm] r208640 - [DAGCombiner] Split up an indexed load if only the base pointer value is live
Hal Finkel
hfinkel at anl.gov
Mon Sep 1 23:42:13 PDT 2014
----- 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.
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
More information about the llvm-commits
mailing list