[llvm] r208640 - [DAGCombiner] Split up an indexed load if only the base pointer value is live
Hal Finkel
hfinkel at anl.gov
Thu Jun 12 17:25:22 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: 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).
> 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.
-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