Page MenuHomePhabricator

[X86] Call removeDeadNode when we're done doing custom isel for mul, div and test
ClosedPublic

Authored by craig.topper on Sep 6 2017, 6:13 PM.

Details

Summary

Once we've done our custom isel for these nodes, I think we should be calling removeDeadNode to prune them out of the DAG. Table driven isel ultimately either calls morphNodeTo which modifies a node and doesn't leave dead nodes. Or it emits new nodes and then calls removeDeadNode as part of Opc_CompleteMatch.

If you run a simple multiply test case like this through llc with -debug you'll see a umul_lohi node get printed as part of the dump for Instruction Selection ends.

define i64 @foo(i64 %a, i64 %b) local_unnamed_addr #0 {
entry:
  %conv = zext i64 %a to i128
  %conv1 = zext i64 %b to i128
  %mul = mul nuw nsw i128 %conv1, %conv
  %shr = lshr i128 %mul, 64
  %conv2 = trunc i128 %shr to i64
  ret i64 %conv2
}

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Sep 6 2017, 6:13 PM
niravd accepted this revision.Sep 8 2017, 7:55 PM

Yet another case of unused nodes being left behind and causing degradation. We should change the behavior of the DAG ReplaceAllUses-ish functions to check if the old source node becomes unused and if so delete the node. This would obviate the need for this patch and catch the lion share of these sorts of issues but will cause assertions in cases a bunch of cases (which is a better situation than the current silent degradation). I have a first-cut patch (D36643) which fixes just the DAGCombiner CombineTo function and all of the assertion failures from update ordering / double deletions.

In any case, this LGTM.

This revision is now accepted and ready to land.Sep 8 2017, 7:55 PM
This revision was automatically updated to reflect the committed changes.