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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:463
       SoftenFloatResult(Op.getNode(), 0);
+      Op.getNode()->setNodeId(Processed);
+    }
----------------
fhahn wrote:
> hfinkel wrote:
> > 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?
> 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.
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?


https://reviews.llvm.org/D28195