This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Add lround/llround builtins
ClosedPublic

Authored by zatrazz on May 1 2019, 11:20 AM.

Details

Summary

This patch add the ISD::LROUND and ISD::LLROUND along with new
intrinsics. The changes are straightforward as for other
floating-point rounding functions, with just some adjustments
required to handle the return value being an interger.

The idea is to optimize lround/llround generation for AArch64
in a subsequent patch. Current semantic is just route it to libm
symbol.

Diff Detail

Event Timeline

zatrazz created this revision.May 1 2019, 11:20 AM
craig.topper added inline comments.
llvm/docs/LangRef.rst
12354

How can this support vector if the result type is hardcoded to i32/i64?

craig.topper added inline comments.May 1 2019, 11:38 AM
llvm/lib/CodeGen/TargetLoweringBase.cpp
717

I wonder if it wouldn't make more sense to index the operation action table by the FP type rather than the result type.

zatrazz updated this revision to Diff 198024.May 3 2019, 8:49 AM

Updated patch based on previous comments. The changes from the previous version are:

  • Remove support vector types from documentation of the new instrinsics.
  • Index the new operations based on input argument rather than return ones.
  • Added tests for i32 variant (lround for long int being i32).
  • Added support and tests for soft-float lowering.
zatrazz updated this revision to Diff 198065.May 3 2019, 12:43 PM

In fact, it turned out that indexing the new lround/llround based on
input argument didn't really make it correctly handled as an expanded
operation. I will dig into exactly why backend is not selecting correctly
based on input argument, so I change it back to previous indexing by
return type.

I have clean up the testscase to remove duplicated entries and add a
x86_64 one so the code path to lower to libcalls will still be stressed
(since the idea for AArch64 is optimize to specific instructions).

To use the FP type you need to add the new ISD opcodes to the switch in SelectionDAGLegalize::LegalizeOp that assigns the Action variable. You'll need to call TLI.getOperationAction on the first operand type.

zatrazz updated this revision to Diff 198318.May 6 2019, 11:47 AM

Updated patch based on the previous comment. The main change are:

  • Add the new ISD on SelectionDAGLegalize::LegalizeOp.
  • Index both lround/llround by returned type.
craig.topper added inline comments.May 6 2019, 10:35 PM
llvm/docs/LangRef.rst
12354

Add 'type' after 'floating-point'.

12395

floating-point type.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2148

Drop the isSigned argument like ExpandFPLibCall

llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
775 ↗(On Diff #198318)

Why is only LROUND handled here? Shouldn't the floating point handling need to be the same regardless of the integer type?

craig.topper added inline comments.May 6 2019, 10:43 PM
llvm/lib/CodeGen/TargetLoweringBase.cpp
701

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

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.

zatrazz marked 10 inline comments as done.May 7 2019, 2:34 PM

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.

llvm/docs/LangRef.rst
12354

Ack.

12395

Ack.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2148

Ack.

llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
775 ↗(On Diff #198318)

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.

llvm/lib/CodeGen/TargetLoweringBase.cpp
701

Ack.

zatrazz updated this revision to Diff 198532.May 7 2019, 2:37 PM
zatrazz marked 5 inline comments as done.

Updated patch based on previous comments. The changes are:

  • Fix documentation missing words.
  • Removed isSigned from ExpandFPLibCall.
  • Added LLROUND along with a proper tests (for mips64) for soft-float.
  • Moved TargetLoweringBase::initActions action for f80 on x86 code.
  • Added lround.i32 for i686 tests using utils/update_llc_test_checks.py.
craig.topper added inline comments.May 7 2019, 3:05 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2164

Doesn't ExpandFPLibCall pass false for the last argument? We should probably do the same here.

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2491 ↗(On Diff #198532)

Mark that bool flag as irrelevant like ExpandIntRes_FP_TO_UINT and ExpandIntRes_FP_TO_SINT?

zatrazz updated this revision to Diff 198703.May 8 2019, 11:38 AM
zatrazz marked an inline comment as done.

Updated patch based on previous review.

This revision is now accepted and ready to land.May 8 2019, 11:51 AM
craig.topper requested changes to this revision.May 8 2019, 12:01 PM

Should we have a PowerPC test for ppcf128?

This revision now requires changes to proceed.May 8 2019, 12:01 PM

Should we have a PowerPC test for ppcf128?

I will update the patch to add one.

zatrazz updated this revision to Diff 198727.May 8 2019, 2:29 PM

Updated patch based on the previous comment. The main change is the IBM long double tests which also required both SoftenFloatOp_LROUND and SoftenFloatOp_LLROUND implementation to handle the type correctly.

This revision is now accepted and ready to land.May 10 2019, 8:41 AM
zatrazz closed this revision.May 16 2019, 6:23 AM