[DAGCombine] SimplifyNodeWithTwoResults - ensure same legalization for LO/HI operands (PR21207)

[RKSimon]

Summary

Consistently use (!LegalOperations || isOperationLegalOrCustom) for all node pairs.

[niravd]

I don't think that need to avoid custom operations post-legalization. The documentation doesn't seem to address this, but as I understand it, Custom is Legal but with non-standard lowering and we just happen to lower custom node in legalize as an easy way to sure we've legalized the DAG.

Can we solve the inconsistency the other way and allow custom in the other cases (L3568, L3584, L3594)?

[RKSimon]

In D53478#1270752, @niravd wrote:

I don't think that need to avoid custom operations post-legalization. The documentation doesn't seem to address this, but as I understand it, Custom is Legal but with non-standard lowering and we just happen to lower custom node in legalize as an easy way to sure we've legalized the DAG.

I don't think we have any other cases in DAGCombiner where we use !LegalOperations || TLI.isOperationLegalOrCustom

[niravd]

which supports my supposition, but "hasOperation" seems to match your interpretation (maybe that's the right check here).

The initial PR21207 seems to only discuss reverting this particular case and states there are other cases in DAGCombiner (and I confirmed there are a few scattered about) .

If we return a custom node post-legalization it should be legalized into a non-custom node, so modulo causing an infinite chain of DAG changes (which we don't have any formal means of preventing anyways) shouldn't returning custom generally be okay? Do you have access to the OOT backend that's triggered PR21207?

(Side note: The 3rd and 4th optimization cases in this function looks redundant with the first two)

In D53478#1270788, @RKSimon wrote:

In D53478#1270752, @niravd wrote:

I don't think that need to avoid custom operations post-legalization. The documentation doesn't seem to address this, but as I understand it, Custom is Legal but with non-standard lowering and we just happen to lower custom node in legalize as an easy way to sure we've legalized the DAG.

I don't think we have any other cases in DAGCombiner where we use !LegalOperations || TLI.isOperationLegalOrCustom

[craig.topper]

DAG combine hasn't always performed legalization on the fly after LegalizeDAG. So there used to only be one chance to legalize.

[RKSimon]

Consistently use isOperationLegalOrCustom - I'll then close PR21207 as invalid now that we have legalization on the fly.

[niravd]

LGTM.

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 
 /// Perform optimizations common to nodes that compute two values. LoOp and HiOp
 /// give the opcodes for the two computations that are being performed. Return
 /// true if a simplification was made.
 SDValue DAGCombiner::SimplifyNodeWithTwoResults(SDNode *N, unsigned LoOp,
                                                 unsigned HiOp) {
   // If the high half is not needed, just compute the low half.
   bool HiExists = N->hasAnyUseOfValue(1);
-  if (!HiExists &&
-      (!LegalOperations ||
+  if (!HiExists && (!LegalOperations ||
                     TLI.isOperationLegalOrCustom(LoOp, N->getValueType(0)))) {
     SDValue Res = DAG.getNode(LoOp, SDLoc(N), N->getValueType(0), N->ops());
     return CombineTo(N, Res, Res);
   }
 
   // If the low half is not needed, just compute the high half.
   bool LoExists = N->hasAnyUseOfValue(0);
-  if (!LoExists &&
-      (!LegalOperations ||
-       TLI.isOperationLegal(HiOp, N->getValueType(1)))) {
+  if (!LoExists && (!LegalOperations ||
+                    TLI.isOperationLegalOrCustom(HiOp, N->getValueType(1)))) {
     SDValue Res = DAG.getNode(HiOp, SDLoc(N), N->getValueType(1), N->ops());
     return CombineTo(N, Res, Res);
   }
 
   // If both halves are used, return as it is.
   if (LoExists && HiExists)
     return SDValue();
 
   // If the two computed results can be simplified separately, separate them.
   if (LoExists) {
     SDValue Lo = DAG.getNode(LoOp, SDLoc(N), N->getValueType(0), N->ops());
     AddToWorklist(Lo.getNode());
     SDValue LoOpt = combine(Lo.getNode());
     if (LoOpt.getNode() && LoOpt.getNode() != Lo.getNode() &&
         (!LegalOperations ||
-         TLI.isOperationLegal(LoOpt.getOpcode(), LoOpt.getValueType())))
+         TLI.isOperationLegalOrCustom(LoOpt.getOpcode(), LoOpt.getValueType())))
       return CombineTo(N, LoOpt, LoOpt);
   }
 
   if (HiExists) {
     SDValue Hi = DAG.getNode(HiOp, SDLoc(N), N->getValueType(1), N->ops());
     AddToWorklist(Hi.getNode());
     SDValue HiOpt = combine(Hi.getNode());
     if (HiOpt.getNode() && HiOpt != Hi &&
         (!LegalOperations ||
-         TLI.isOperationLegal(HiOpt.getOpcode(), HiOpt.getValueType())))
+         TLI.isOperationLegalOrCustom(HiOpt.getOpcode(), HiOpt.getValueType())))
       return CombineTo(N, HiOpt, HiOpt);
   }
 
   return SDValue();
 }
 
 SDValue DAGCombiner::visitSMUL_LOHI(SDNode *N) {
   if (SDValue Res = SimplifyNodeWithTwoResults(N, ISD::MUL, ISD::MULHS))