[PATCH] D61390: [CodeGen] Add lround/llround builtins

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz marked 10 inline comments as done.
zatrazz added a comment.

In D61390#1492891 <https://reviews.llvm.org/D61390#1492891>, @craig.topper wrote:

> Can you test lround.i32 with -mtriple=i686-unknown with and without -mattr=sse2.
>
> Use utils/update_llc_test_checks.py to generate the checks for at least X86. Try it on the other targets too if it works for them.


I have added a lround.i32 for i686 as you asked using utils/update_llc_test_checks.py.  I will update the patch.



================
Comment at: llvm/docs/LangRef.rst:12354
+This is an overloaded intrinsic. You can use ``llvm.lround`` on any
+floating-point. Not all targets support all types however.
+
----------------
craig.topper wrote:
> Add 'type' after 'floating-point'.
Ack.


================
Comment at: llvm/docs/LangRef.rst:12395
+This is an overloaded intrinsic. You can use ``llvm.llround`` on any
+floating-point. Not all targets support all types however.
+
----------------
craig.topper wrote:
> floating-point type.
Ack.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2150
+/// lround and its variant).
+SDValue SelectionDAGLegalize::ExpandArgFPLibCall(SDNode* Node, bool isSigned,
+                                                 RTLIB::Libcall Call_F32,
----------------
craig.topper wrote:
> Drop the isSigned argument like ExpandFPLibCall
Ack.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:775
   case ISD::FP_TO_UINT:  Res = SoftenFloatOp_FP_TO_XINT(N); break;
+  case ISD::LROUND:      Res = SoftenFloatOp_LROUND(N); break;
   case ISD::SELECT:      Res = SoftenFloatOp_SELECT(N); break;
----------------
craig.topper wrote:
> Why is only LROUND handled here? Shouldn't the floating point handling need to be the same regardless of the integer type?
It should and it haven't added because the test coverage didn't actually hit the requirement. With the updated patch that check for 64-bit soft-float (Mips64) I have added the LLROUND handling as well.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:700
   // These library functions default to expand.
-  for (MVT VT : {MVT::f32, MVT::f64, MVT::f128}) {
+  for (MVT VT : {MVT::f32, MVT::f64, MVT::f80, MVT::f128}) {
     setOperationAction(ISD::FCBRT,      VT, Expand);
----------------
craig.topper wrote:
> Instead of adding f80 here which only X86 needs, just add the new nodes in the same spot as FLOG, FLOG2, FLOG10, etc. in X86ISelLowering.cpp
Ack.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61390/new/

https://reviews.llvm.org/D61390