Page MenuHomePhabricator

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

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



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

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.

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:


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

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

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


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