Page MenuHomePhabricator

[DAGCombiner] Require ninf for division estimation
ClosedPublic

Authored by qiucf on May 26 2020, 2:21 AM.

Details

Summary

This is similar to D76853. Current implementation of division estimation isn't correct for 1.0/0.0 (result is nan, not expected inf).

And this change exposes a potential infinite loop: in combineRepeatedFPDivisors, we use isConstOrConstSplatFP to look up if the divisor is some constant. But it doesn't work after legalized on some platforms. This patch restricts the method to act before LegalDAG.

Diff Detail

Event Timeline

qiucf created this revision.May 26 2020, 2:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 2:21 AM
spatel added inline comments.May 27 2020, 1:08 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13101

Do we need to check the target option here? If all tests are updated to use node-level fast-math-flags, then I would prefer not to include legacy/deprecated predicate.

arsenm added inline comments.May 27 2020, 3:18 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13101

Probably should keep checking the attribute for consistency. Plus the attributes have the nice feature that you can apply them to bitcode functions when linking, but we don't have propagate FMF

I think, this is the right fix. Notice that, even with this fix, we still have issue when the input is denormal floating point. We might have to add some test before doing the software expansion as what we did for sqrt. But this check may hurt the performance.

cat a.C

#include <stdio.h>
double __attribute__((noinline)) foo(double a, double b) {
	return a/b;
}

int main() {
	printf("%g\n", foo(1.0, 2.2250738585072009e-309));
	return 0;
}

clang a.C -Ofast;./a.out

nan

qiucf added a comment.May 28 2020, 1:43 AM

@steven.zhang : We might have to add some test before doing the software expansion as what we did for sqrt. But this check may hurt the performance.

Yes.. On PowerPC, we have a 'test' instruction to determine whether two numbers are suitable for division estimation or not. (extremely big/small numbers, x is nan/inf, y is nan/inf/zero...) And I think we don't need this flag requirement after such exploitation. (maybe move the check inside BuildDivEstimate and add a hook?)

spatel accepted this revision.May 28 2020, 5:59 AM

LGTM - if you want to move the flags check inside BuildDivEstimate() to work better with D80706, either way seems fine to me.

This revision is now accepted and ready to land.May 28 2020, 5:59 AM
This revision was automatically updated to reflect the committed changes.