Page MenuHomePhabricator

[FPEnv] Fix chain handling for fpexcept.strict nodes
ClosedPublic

Authored by uweigand on Jan 7 2020, 8:54 AM.

Details

Summary

As pointed out in D70913, it turns out we need to ensure that fpexcept.strict nodes are not optimized away even if the result is unused. To do that, we need to chain them into the block's terminator nodes, like already done for PendingExcepts.

This patch adds two new lists of pending chains, PendingConstrainedFP and PendingConstrainedFPStrict to hold constrained FP intrinsic nodes without and with fpexcept.strict markers. This allows not only to solve the above problem, but also to relax chains a bit further by no longer flushing all FP nodes before a store or other memory access. (They are still flushed before nodes with other side effects.)

Note that this patch currently introduces test case failures:

LLVM :: CodeGen/X86/vec-strict-cmp-128.ll
LLVM :: CodeGen/X86/vec-strict-cmp-256.ll
LLVM :: CodeGen/X86/vec-strict-cmp-512.ll
LLVM :: CodeGen/X86/vec-strict-fptoint-128.ll
LLVM :: CodeGen/X86/vec-strict-fptoint-256.ll
LLVM :: CodeGen/X86/vec-strict-fptoint-512.ll

These appear to be caused by problems handling the outgoing chains of some strictfp nodes in the X86 back-end; these problems were unnoticed before this patch because the outgoing chains were simply unused with those simple tests otherwise.

It may be that this is actually the same problem to be addressed by D72224; @craig.topper , maybe you can have a look?

Diff Detail

Event Timeline

uweigand created this revision.Jan 7 2020, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2020, 8:54 AM
craig.topper added inline comments.Jan 7 2020, 12:13 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1081

Can we just use PendingExports.append here? I think it will take care of the reserve. I think even insert will probably take care of the reserve. Might make sense for the code in getRoot() too unless you're concerned about 2 reserves in 2 separate append calls.

Looks like D72224 fixes those failures and removes at least one of the diffs in this patch.

llvm/test/CodeGen/X86/vector-constrained-fp-intrinsics.ll
7472–7500

This diff is recovered with D72224

uweigand updated this revision to Diff 236804.Jan 8 2020, 6:02 AM

Use SmallVector::append instead of ::insert.

uweigand marked an inline comment as done.Jan 8 2020, 6:03 AM

Looks like D72224 fixes those failures and removes at least one of the diffs in this patch.

Ah, good -- thanks for testing!

Maybe the best would then be to wait for D72224 to be committed first.

pengfei added a subscriber: pengfei.Jan 8 2020, 5:13 PM
uweigand updated this revision to Diff 237400.Jan 10 2020, 11:31 AM
uweigand retitled this revision from [FPEnv][WIP] Fix chain handling for fpexcept.strict nodes to [FPEnv] Fix chain handling for fpexcept.strict nodes.

Updated patch now that D72224 has landed. At this point, there are no test suite failures remaining.

craig.topper added inline comments.Jan 11 2020, 5:53 PM
llvm/test/CodeGen/X86/fp128-libcalls-strict.ll
1131

This looks like a bug of some sort. I'll see if I can track it down.

craig.topper added inline comments.Jan 11 2020, 5:54 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7020

This is throwing a warning that the default is redundant in a fully covered switch.

craig.topper added inline comments.Jan 11 2020, 8:51 PM
llvm/test/CodeGen/X86/fp128-libcalls-strict.ll
1131

I think I've fixed this or at least hacked around it for now. Can you rebase this?

uweigand updated this revision to Diff 237540.Jan 12 2020, 8:57 AM

Address review comments and rebase.

uweigand marked 2 inline comments as done.Jan 12 2020, 9:00 AM

Looks like the remaining test case changes are strictly due to scheduling.

This revision is now accepted and ready to land.Jan 12 2020, 12:29 PM
This revision was automatically updated to reflect the committed changes.

It turns out this patch introduced a regression with chain handling in code mixing constrained intrinsics and memory operations.

This is now fixed here in commit 81ee484: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200113/729980.html

Using that commit, I'm now able to build and run all of test-suite on SystemZ with the -ftrapping-math flag.