This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] New sign-of-zero compliant patterns for fabs folding
AbandonedPublic

Authored by mike.dvoretsky on Mar 5 2018, 6:52 AM.

Details

Reviewers
spatel
bogner
Summary

Patterns that were folded into fabs in DAGCombiner with the no-NaNs fast-math flag were semantically incorrect, as they should actually produce -0.0 on one of the zero inputs. This patch adds a no-signed-zeros check around those patterns as well as additional sign-of-zero compliant patterns that do not require this check.

Fixes https://bugs.llvm.org/show_bug.cgi?id=36600.

Diff Detail

Event Timeline

mike.dvoretsky created this revision.Mar 5 2018, 6:52 AM
mike.dvoretsky created this object with visibility "mike.dvoretsky (Mikhail Dvoretckii)".
mike.dvoretsky created this object with edit policy "mike.dvoretsky (Mikhail Dvoretckii)".
mike.dvoretsky edited the summary of this revision. (Show Details)Mar 5 2018, 9:10 AM
mike.dvoretsky changed the visibility from "mike.dvoretsky (Mikhail Dvoretckii)" to "Public (No Login Required)".
mike.dvoretsky changed the edit policy from "mike.dvoretsky (Mikhail Dvoretckii)" to "All Users".
mike.dvoretsky added a subscriber: ashlykov.
spatel added a subscriber: spatel.Mar 5 2018, 10:54 AM

I fixed the test file to check all output here:
rL326735
Use the script to auto-generate check lines for any tests that you are adding.

Please rebase and upload the patch with full context:
https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
16964

We're checking the flags on a node, but there is no regression test with 'nsz' FMF on the IR?

16982–16983

Use 'auto' with 'dyn_cast':
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

...although what happens with vector types? Are they handled on a different path?

Rebased and reuploaded with full context. Added code to propagate nsz from IR to DAG for the fneg node, as well as to retain flags during vector type legalization. Added tests for flag use on nodes and for vector type fabs folding.

mike.dvoretsky set the repository for this revision to rL LLVM.

Style correction.

mike.dvoretsky marked 2 inline comments as done.Mar 6 2018, 8:42 AM
mike.dvoretsky retitled this revision from [DAGCombiner] New sign-of-zero compliant patterns for fabs folding to [SelectionDAG] New sign-of-zero compliant patterns for fabs folding.Mar 9 2018, 12:50 AM

Added TargetOptions checks to get nsz from function attributes. Added a test case for that.

Corrected an error in test7.

What do you think of my suggestion from D44550 - can we just remove this code from SDAG now that we canonicalize the IR?

I agree with this, and I think it should be a new revision rather that this one. If you agree, I'll declare this one abandoned and leave it as is, then make a new one that just removes the buggy code from DAGCombiner.

I agree with this, and I think it should be a new revision rather that this one. If you agree, I'll declare this one abandoned and leave it as is, then make a new one that just removes the buggy code from DAGCombiner.

Sounds good.

mike.dvoretsky abandoned this revision.Mar 19 2018, 9:10 AM