Page MenuHomePhabricator

[PowerPC] Add exception constraint to FP rounding operations
ClosedPublic

Authored by qiucf on Jul 3 2019, 11:10 PM.

Details

Summary

The concept of strict floating point nodes was added into LLVM in https://reviews.llvm.org/D55506.

This is the second follow-up patch to add round/trunc/extend constraint fp intrinsics support for PowerPC target.

Diff Detail

Event Timeline

wuzish created this revision.Jul 3 2019, 11:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 11:10 PM
ajwock added a subscriber: ajwock.Jul 12 2019, 12:41 PM
jsji added a reviewer: Restricted Project.Aug 27 2019, 7:56 PM
qiucf commandeered this revision.Oct 11 2019, 7:27 PM
qiucf added a reviewer: wuzish.
qiucf planned changes to this revision.Jan 6 2020, 1:32 AM

This patch is too large to review and 'remaining parts' is confusing. It's better to split this one into several patches (round, extend/trunc, sqrt, fma, etc.) and push them into a review stack.

qiucf updated this revision to Diff 238171.Jan 14 2020, 10:41 PM
qiucf retitled this revision from [PowerPC] Add constraint fp support about exception part for remaining operations to [PowerPC] Add exception constraint to FP rounding operations.

Rebase and fix test errors

Unit tests: pass. 61901 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

qiucf updated this revision to Diff 251008.Mar 18 2020, 1:02 AM
qiucf edited the summary of this revision. (Show Details)

Merge tests and update summary.

qiucf updated this revision to Diff 253487.Mar 29 2020, 9:47 PM

Update to resolve conflicts

steven.zhang added inline comments.Mar 29 2020, 10:24 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
375

Why we have this lines of changes here ?

llvm/test/CodeGen/PowerPC/build-vector-tests.ll
1514 ↗(On Diff #253487)

Could you please explain more about this extra instruction produced ?

qiucf updated this revision to Diff 254427.Apr 1 2020, 10:22 PM
qiucf marked 3 inline comments as done.

Rebase after D77208

llvm/test/CodeGen/PowerPC/build-vector-tests.ll
1514 ↗(On Diff #253487)

It should be fixed after D77208

steven.zhang added inline comments.Apr 28 2020, 9:49 PM
llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
5927–5931

This looks like a deg. Please explain more about how this happens.

qiucf marked an inline comment as done.May 12 2020, 2:28 AM
qiucf added inline comments.
llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
5927–5931

This is a reasonable result (for this case). Since arguments to ceil is constant 1.5, result as 2.0 is provided even before DAG built.

Without this patch, these unused node (frip) would be dropped after isel, before list scheduling. However, every strict-fp node will have another operand for its chain. So they won't be dropped before list scheduling.

Maybe using variable as operand hits more about what we intend to test.

steven.zhang added inline comments.May 14 2020, 3:57 AM
llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
6255

This is not right. It seems that, we are still returning the rounded constant with compiler folding which didn't respect the round mode. What we did now, is just re-run the calculation to make sure that, the hw status register is updated correctly. But the result is still wrong.

qiucf marked an inline comment as done.May 18 2020, 8:46 PM
qiucf added inline comments.
llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
6255

D72930 introduced const-folding for these rounding ops. That looks reasonable since it doesn't depend on rounding mode and won't raise exceptions.

So it seems to be safe to delete these strict-fp ops? (they have extra operand to chain so they can't be removed automatically) Will this be an improvement?

cc: @kpn @andrew.w.kaylor

llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
6255

They are not folded in all cases. If the input values can raise an exception (such a invalid), the operation is not folded.

steven.zhang accepted this revision.May 19 2020, 8:04 PM

LGTM. But please hold on for several days in case someone has more comments.

llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
6255

ok. We can still go with this patch and revisit this case later to see why frin is still there.

This revision is now accepted and ready to land.May 19 2020, 8:04 PM

I looked through this and it seems fine to me.
I'm not clear about the extraneous frin in vector-constrained-fp-intrinsics.ll, but if it is extraneous I agree we can proceed with this and deal with it in a subsequent patch.

qiucf updated this revision to Diff 266723.May 27 2020, 8:13 PM

Update with subtle change to rebase before commit. Thanks to all reviewers!

qiucf updated this revision to Diff 266725.May 27 2020, 8:18 PM
Esme added a subscriber: Esme.Jun 11 2020, 1:19 AM
Esme added inline comments.
llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
6255

I think results here are reasonable. In EarlyCSE, constrained rounding ops may be constant-folded if they don't raise FP exceptions, but the dead code will not be deleted because they are still treated as mayThrow. Since these ops are matched to HW instructions with the flag mayRaiseFPException now, it should be reasonable to not eliminate them.

This revision was automatically updated to reflect the committed changes.