[llvm] r208640 - [DAGCombiner] Split up an indexed load if only the base pointer value is live
Adam Nemet
anemet at apple.com
Wed May 28 09:57:59 PDT 2014
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
More information about the llvm-commits
mailing list