Page MenuHomePhabricator

[DAGCombine] Don't delete the node if it has uses immediately
ClosedPublic

Authored by steven.zhang on Aug 27 2020, 1:43 AM.

Details

Summary

This is the follow up patch for https://reviews.llvm.org/D86183 as we miss to delete the node if NegX == NegY, which has use after we create the node.

if (NegX && (CostX <= CostY)) {
  Cost = std::min(CostX, CostZ);
  RemoveDeadNode(NegY);
  return DAG.getNode(Opcode, DL, VT, NegX, Y, NegZ, Flags);  #<-- NegY is used here if NegY == NegX.
}

Diff Detail

Event Timeline

steven.zhang created this revision.Aug 27 2020, 1:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2020, 1:43 AM
steven.zhang requested review of this revision.Aug 27 2020, 1:43 AM
steven.zhang edited the summary of this revision. (Show Details)
steven.zhang edited the summary of this revision. (Show Details)
spatel added inline comments.Aug 27 2020, 10:53 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5823

I think it would be clearer and less code duplication to put the 'if' check into the lambda, so something like:

auto RemoveDeadNode = [&](SDValue N, SDValue Other) {
  if (N && N.getNode()->use_empty() && N != Other)
    DAG.RemoveDeadNode(N.getNode());
};

Then call it with something like this:

RemoveDeadNode(NegX, NegY);
llvm/test/CodeGen/PowerPC/fneg.ll
56–62

We should reduce the test to avoid UB and extra operations, so something like this should work?

define double @fneg_no_ice(float %x) {
  %y = fsub fast float 1.0, %x
  %e = fpext float %y to double
  %e2 = fmul double %e, %e
  %e3 = fmul double %e, %e2
  ret double %e3
}
steven.zhang added inline comments.Aug 27 2020, 5:37 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5823

we need also check NegX != NegZ...

steven.zhang added inline comments.Aug 27 2020, 5:54 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5823

So, I think, move it after the getNode() will be more safe and make RemoveDeadNode() semantics clean. Otherwise, we have to check NegY != NegX and NegY != NegZ for FMA.

llvm/test/CodeGen/PowerPC/fneg.ll
56–62

Good point. Yes, it works.

Update test.

spatel accepted this revision.Aug 28 2020, 5:38 AM

LGTM

This revision is now accepted and ready to land.Aug 28 2020, 5:38 AM
This revision was automatically updated to reflect the committed changes.
jsji added a subscriber: jsji.Aug 28 2020, 9:15 AM

Commit on behalf of @steven.zhang to unblock internal build first..

Hi,

I wrote https://bugs.llvm.org/show_bug.cgi?id=47517 about a crash that starts happening with this patch.