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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


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


https://reviews.llvm.org/D28195