Page MenuHomePhabricator

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

Authored by fhahn on Dec 31 2016, 5:16 AM.

Details

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)

Diff Detail

Event Timeline

fhahn updated this revision to Diff 82762.Dec 31 2016, 5:16 AM
fhahn retitled this revision from to [selectiondag] Mark node created in SoftenFloatRes_FP_EXTEND as Processed..
fhahn updated this object.
fhahn added reviewers: t.p.northover, baldrick, olista01.
fhahn added a subscriber: llvm-commits.
RKSimon added a subscriber: RKSimon.Jan 6 2017, 9:25 AM
RKSimon added a subscriber: davide.
fhahn added a comment.Jan 11 2017, 6:20 AM

Ping. Any thoughts on the patch?

The fp128 failures on PR30999 are failing from something similar

fhahn added a comment.Jan 17 2017, 2:39 PM

@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 added a comment.Jan 23 2017, 7:32 AM

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

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 added inline comments.
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 updated this revision to Diff 86508.Jan 31 2017, 3:30 PM
fhahn retitled this revision from [selectiondag] Mark node created in SoftenFloatRes_FP_EXTEND as Processed. to [legalizetypes] Push fp16 -> fp32 extension node to worklist. .
fhahn edited the summary of this revision. (Show Details)
fhahn added inline comments.Jan 31 2017, 3:33 PM
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 added inline comments.Feb 1 2017, 3:07 AM
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 updated this revision to Diff 86606.Feb 1 2017, 3:30 AM

Add AddToWorklist helper function.

fhahn marked an inline comment as done.Feb 1 2017, 3:31 AM
hfinkel accepted this revision.Feb 1 2017, 4:12 AM

LGTM

This revision is now accepted and ready to land.Feb 1 2017, 4:12 AM
fhahn closed this revision.Feb 1 2017, 5:12 AM