Page MenuHomePhabricator

[DAG] Cleanup unused node in SimplifySelectCC.
ClosedPublic

Authored by niravd on Feb 7 2019, 1:10 PM.

Details

Summary

Delete temporarily constructed node uses for analysis after it's use,
holding onto original input nodes. Ideally this would be rewritten
without making nodes, but this appears relatively complex.

Diff Detail

Event Timeline

niravd created this revision.Feb 7 2019, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 1:10 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Can you describe what's happening within SimplifySetCC for the affected test?

RKSimon added inline comments.Mar 1 2019, 12:39 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18677

Could we replace this with a call to SelectionDAG::FoldSetCC()? That should only ever constant fold which AFAICT is what we're after.

RKSimon added inline comments.Mar 1 2019, 12:48 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18677

Actually it looks like there's a tiny bit of commutation code (moves single constant to RHS) that probably needs pulling out of FoldSetCC into getNode - but that's trivial compared to the TLI.SimplifySetCC monster.

RKSimon added inline comments.Mar 11 2019, 8:12 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18677

Confirmed, this gives the same result and is a lot cleaner:

// Determine if the condition we're dealing with is constant.
if (SDValue SCC = DAG.FoldSetCC(VT, N0, N1, CC, DL)) {
  if (auto *SCCC = dyn_cast<ConstantSDNode>(SCC)) {
    // fold select_cc true, x, y -> x
    // fold select_cc false, x, y -> y
    return !SCCC->isNullValue() ? N2 : N3;
  }
}

@niravd Are you still looking at this?

niravd updated this revision to Diff 191107.Mar 18 2019, 9:17 AM

Copy over Simon's suggestion.

It fell off the end of my work queue, but yes.

There's also a natural followup replacing another instance of SimplifySetCC, which seems to have similar wins that I'll put up in a bit.

@niravd Are you still looking at this?

RKSimon accepted this revision.Mar 18 2019, 9:35 AM

LGTM - cheers

This revision is now accepted and ready to land.Mar 18 2019, 9:35 AM
This revision was automatically updated to reflect the committed changes.