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.
Details
Details
- Reviewers
hfinkel nemanjai jsji kbarton uweigand andrew.w.kaylor kpn • wuzish steven.zhang - Group Reviewers
Restricted Project - Commits
- rG13edcd696e08: [PowerPC] Support constrained rounding operations
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
Comment Actions
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
| llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll | ||
|---|---|---|
| 5927–5931 | This looks like a deg. Please explain more about how this happens. | |
| 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. | |
| 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. | |
| 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. | |
Comment Actions
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. | |
Comment Actions
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.
| 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. | |
Why we have this lines of changes here ?