Page MenuHomePhabricator

[SDAG] Add new ISD nodes: ISD::FMINNAN and ISD::FMAXNAN
ClosedPublic

Authored by jmolloy on Jul 16 2015, 7:36 AM.

Details

Reviewers
ab
arsenm
hfinkel
Summary

The intention of these is to be a corollary to ISD::FMINNUM/FMAXNUM,
differing only on how NaNs are treated. FMINNUM returns the non-NaN
input (when given one NaN and one non-NaN), FMINNAN returns the NaN
input instead.

This patch includes support for scalarizing, widening and splitting
vectors, but not expansion or softening. The reason is that these
should never be needed - FMINNAN nodes are only going to be created
in one place (SDAGBuilder::visitSelect) and there we'll check if the
node is legal or custom. I could preemptively add expand and soften
code, but I'm fairly opposed to adding code I can't test. It's bad
enough I can't create tests with this patch, but at least this code
will be exercised by the ARM and AArch64 backends fairly shortly.

Diff Detail

Repository
rL LLVM

Event Timeline

jmolloy updated this revision to Diff 29904.Jul 16 2015, 7:36 AM
jmolloy retitled this revision from to [SDAG] Add new ISD nodes: ISD::FMINNAN and ISD::FMAXNAN.
jmolloy updated this object.
jmolloy added reviewers: arsenm, ab.
jmolloy set the repository for this revision to rL LLVM.
jmolloy added a subscriber: llvm-commits.
arsenm edited edge metadata.Jul 22 2015, 10:51 AM

Do these have a corresponding IR intrinsic like fminnum/fmaxnum because I don't see the SelectionDAGBuilder part?

include/llvm/CodeGen/ISDOpcodes.h
513–517

I think this comment should be split into a more detailed description of just FMINNUM/FMAXNUM, and then a separate comment for how FMINNAN/FMAXNAN differs

Hi Matt,

Do these have a corresponding IR intrinsic like fminnum/fmaxnum because I don't see the SelectionDAGBuilder part?

They don't have intrinsics (although I can create some if you want). I have SDAGBuilder changes in a followup (it generates both fmaxnum and fmaxnan, so I didn't think it belonged in the same diff.

James

Can you please describe the SDAG changes you have in mind; is there some common sequence of nodes involving fminnum/fmaxnum that you expect to turn into these nodes?

jmolloy updated this revision to Diff 30692.Jul 27 2015, 7:43 AM
jmolloy edited edge metadata.

Hi Hal, Matt,

I've addressed Matt's comment in this latest revision. Does it look alright to both of you?

Cheers,

James

hfinkel accepted this revision.Aug 9 2015, 7:32 PM
hfinkel added a reviewer: hfinkel.

LGTM.

include/llvm/CodeGen/ISDOpcodes.h
509

Line is too long.

This revision is now accepted and ready to land.Aug 9 2015, 7:32 PM

Landed in r244581.

jmolloy closed this revision.Aug 12 2015, 8:14 AM