[legalizetypes] Push fp16 -> fp32 extension node to worklist.

[fhahn]

Summary

This way, the type legalization machinery will take care of registering
the result of this node properly.

This patches fixes all failing fp16 test cases with expensive checks.
(CodeGen/ARM/fp16-promote.ll, CodeGen/ARM/fp16.ll, CodeGen/X86/cvt16.ll
CodeGen/X86/soft-fp.ll)

[fhahn]

Ping. Any thoughts on the patch?

[RKSimon]

The fp128 failures on PR30999 are failing from something similar

[fhahn]

@RKSimon Yes I think some of the remaining XDEBUG failures for X86 are due to related issues. I'll try to have a look soon.

[fhahn]

I just checked, this patch fixes the following failing tests with expensive checks:

LLVM :: CodeGen/ARM/fp16-promote.ll
LLVM :: CodeGen/ARM/fp16.ll
LLVM :: CodeGen/Thumb2/float-intrinsics-double.ll
LLVM :: CodeGen/X86/cvt16.ll

[fhahn]

Seems like there haven't been many changes to LegalizeTypes recently. I'm adding more reviewers based on some of the last reviews in the area (from 2015 and 2016), I would appreciate any comments.

[hfinkel]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

463

To some extent, this makes sense. The caller of SoftenFloatResult, DAGTypeLegalizer::run, is also what sets the node ID to Processed.

I wonder if there's a better design here, because I share your uncertainty about all of the relevant legalities being satisfied. It seems like what we're doing is taking

FP_EXTEND f16 x -> fn

and rewriting them as:

FP_EXTEND (FP_EXTEND f16 x -> f32) -> fn

where we're trying to directly recurse on that inner operation. I don't understand why we're trying to do that as opposed to just queueing the new node for processing. Would it work to just do:

Worklist.push_back(Op.getNode());

instead of calling SoftenFloatResult?

[fhahn]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

463

Thank you very much for having a look! It's indeed possible to push the node to the worklist and let the legalization machinery take care of it.

I had to remove the call

Op = GetSoftenedFloat(Op);

because now that SoftenFloatResult isn't called anymore, GetSoftenedFloat will fail (I think it should only be called in the cases we called SoftenFloatResult previously.

[hfinkel]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

463

Great, thanks!

Given that we also need to call setNodeId to make this work, can/should we add an addToWorklist member function that encapsulates the Worklist.push_back + setNodeId?

[fhahn]

Add AddToWorklist helper function.

[hfinkel]

LGTM

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

SDValue DAGTypeLegalizer::SoftenFloatRes_FP_EXTEND(SDNode *N) {
   SDValue Op = N->getOperand(0);
 
   // There's only a libcall for f16 -> f32, so proceed in two stages. Also, it's
   // entirely possible for both f16 and f32 to be legal, so use the fully
   // hard-float FP_EXTEND rather than FP16_TO_FP.
   if (Op.getValueType() == MVT::f16 && N->getValueType(0) != MVT::f32) {
     Op = DAG.getNode(ISD::FP_EXTEND, SDLoc(N), MVT::f32, Op);
     if (getTypeAction(MVT::f32) == TargetLowering::TypeSoftenFloat)
-      SoftenFloatResult(Op.getNode(), 0);
+      AddToWorklist(Op.getNode());
   }
 
   if (getTypeAction(Op.getValueType()) == TargetLowering::TypePromoteFloat) {
     Op = GetPromotedFloat(Op);
     // If the promotion did the FP_EXTEND to the destination type for us,
     // there's nothing left to do here.
     if (Op.getValueType() == N->getValueType(0)) {
       return BitConvertToInteger(Op);
     }
   }
 
   RTLIB::Libcall LC = RTLIB::getFPEXT(Op.getValueType(), N->getValueType(0));
-  if (getTypeAction(Op.getValueType()) == TargetLowering::TypeSoftenFloat)
-    Op = GetSoftenedFloat(Op);
   assert(LC != RTLIB::UNKNOWN_LIBCALL && "Unsupported FP_EXTEND!");
   return TLI.makeLibCall(DAG, LC, NVT, Op, false, SDLoc(N)).first;
 }
 
 // FIXME: Should we just use 'normal' FP_EXTEND / FP_TRUNC instead of special
 // nodes?
 SDValue DAGTypeLegalizer::SoftenFloatRes_FP16_TO_FP(SDNode *N) {
   EVT MidVT = TLI.getTypeToTransformTo(*DAG.getContext(), MVT::f32);

lib/CodeGen/SelectionDAG/LegalizeTypes.h

private:
   /// The bit vector is widened with zeroes when WithZeroes is true.
   SDValue WidenTargetBoolean(SDValue Bool, EVT ValVT, bool WithZeroes = false);
 
   void ReplaceValueWith(SDValue From, SDValue To);
   void SplitInteger(SDValue Op, SDValue &Lo, SDValue &Hi);
   void SplitInteger(SDValue Op, EVT LoVT, EVT HiVT,
                     SDValue &Lo, SDValue &Hi);
 
+  void AddToWorklist(SDNode *N) {
+    N->setNodeId(ReadyToProcess);
+    Worklist.push_back(N);
+  }
+
   //===--------------------------------------------------------------------===//
   // Integer Promotion Support: LegalizeIntegerTypes.cpp
   //===--------------------------------------------------------------------===//
 
   /// Given a processed operand Op which was promoted to a larger integer type,
   /// this returns the promoted value. The low bits of the promoted value
   /// corresponding to the original type are exactly equal to Op.
   /// The extra bits contain rubbish, so the promoted value may need to be zero-

lib/CodeGen/SelectionDAG/LegalizeTypes.cpp

bool DAGTypeLegalizer::run() {
   // done.  Set it to null to avoid confusion.
   DAG.setRoot(SDValue());
 
   // Walk all nodes in the graph, assigning them a NodeId of 'ReadyToProcess'
   // (and remembering them) if they are leaves and assigning 'Unanalyzed' if
   // non-leaves.
   for (SDNode &Node : DAG.allnodes()) {
     if (Node.getNumOperands() == 0) {
-      Node.setNodeId(ReadyToProcess);
-      Worklist.push_back(&Node);
+      AddToWorklist(&Node);
     } else {
       Node.setNodeId(Unanalyzed);
     }
   }
 
   // Now that we have a set of nodes to process, handle them all.
   while (!Worklist.empty()) {
 #ifndef EXPENSIVE_CHECKS