This is an archive of the discontinued LLVM Phabricator instance.

[Intrinsic] Add llvm.minimum and llvm.maximum instrinsic functions
ClosedPublic

Authored by tlively on Oct 1 2018, 7:52 PM.

Details

Summary

These new intrinsics have the semantics of the minimum and maximum
operations specified by the latest draft of IEEE 754-2018. Unlike
llvm.minnum and llvm.maxnum, these new intrinsics propagate NaNs and
always treat -0.0 as less than 0.0. minimum and maximum lower
directly to the existing fminnan and fmaxnan ISel DAG nodes. It is
safe to reuse these DAG nodes because before this patch were only
emitted in situations where there were known to be no NaN arguments or
where NaN propagation was correct and there were known to be no zero
arguments. I know of only four backends that lower fminnan and
fmaxnan: WebAssembly, ARM, AArch64, and SystemZ, and each of these
lowers fminnan and fmaxnan to instructions that are compatible with
the IEEE 754-2018 semantics.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Oct 1 2018, 7:52 PM
javed.absar added inline comments.Oct 2 2018, 4:04 AM
docs/LangRef.rst
11638 ↗(On Diff #167874)

Does this need a para on sNaN and qNaN behaviour (as above for llvm.max) ?

I agree that this seems fine for SystemZ.

tlively added inline comments.Oct 2 2018, 10:52 AM
docs/LangRef.rst
11638 ↗(On Diff #167874)

I don't think so. llvm.maxnum and llvm.minnum need that paragraph because backends may have the unusual responsibility to quiet input NaNs for those operations in order to match the intrinsic semantics. That is because the minNum and maxNum in IEEE 754-2008 are exceptional in their handling of sNaNs and qNaNs. In contrast, these new intrinsics have the standard NaN propagation behavior for both sNaNs and qNaNs. Backends shouldn't need to take any additional action to canonicalize NaNs to get the right results.

arsenm added a subscriber: arsenm.Oct 2 2018, 7:43 PM
arsenm added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5588 ↗(On Diff #167874)

Either as a dependency or an after-step, I think the DAG nodes should be renamed to match the intrinsic

tlively added inline comments.Oct 3 2018, 10:20 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5588 ↗(On Diff #167874)

I'd be happy to do this in a follow-on. This makes sense to me because the names minnan and maxnan imply that the only difference from minnum and maxnum is NaN propagation, which is not correct.

Bit of formatting nits

docs/LangRef.rst
11564 ↗(On Diff #167874)

One more ^ at the end

11602 ↗(On Diff #167874)

One more ^ at the end

11613 ↗(On Diff #167874)
  • double whitespaces between float and %Val1
  • stray l after %Val1
11615 ↗(On Diff #167874)

double whitespaces between x86_fp80 and %Val0 / x86_fp80 and %Val1

11617 ↗(On Diff #167874)

double whitespaces between ppc_fp128 and %Val0 / ppc_fp128 and %Val1

tlively updated this revision to Diff 168344.Oct 4 2018, 11:59 AM
tlively marked 5 inline comments as done.
  • Formatting
javed.absar accepted this revision.Oct 12 2018, 1:21 AM
This revision is now accepted and ready to land.Oct 12 2018, 1:21 AM
This revision was automatically updated to reflect the committed changes.