This is an archive of the discontinued LLVM Phabricator instance.

[DAG] fold FP binops with undef operands to NaN
ClosedPublic

Authored by spatel on May 17 2018, 12:46 PM.

Details

Summary

This is the FP sibling of D43141 with the corresponding IR change in rL327212.

We can't propagate undef here because if a variable operand is a NaN, these binops must propagate NaN. Neither global nor node-level fast-math makes a difference. If we have 'nnan', I think later folds can turn the NaN into undef.

The tests in X86/fp-undef.ll are meant to be the definitive verification for these folds - everything reduces identically now.

The other test changes are collateral damage that I wasn't sure what to do with (see the many test changes I committed in the last day for attempts to preserve functionality independently of this change).

Let me describe those test diffs, and someone with a better understanding may be able to fix those tests:
AArch64/fcvt_combine.ll - we constant folded the fmul, not sure why the expectation was different
AMDGPU/mad-mix-lo.ll - don't know anything about what's happening here
NVPTX/implicit-def.ll - this isn't testing what it intended to test - delete the file.
X86/pr23103.ll - this isn't testing what it intended to test, but I don't know how to do that...could just delete the file?
X86/vector-reduce-fadd.ll and X86/vector-reduce-fmul.ll - I think all of these diffs are the unexpected case where the accumulator param is supposed to be used because it's a strict reduction, but we're passing in undef. Just verifies that we don't crash?
http://llvm.org/docs/LangRef.html#llvm-experimental-vector-reduce-fadd-intrinsic

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.May 17 2018, 12:46 PM
jlebar added a comment.EditedMay 17 2018, 12:58 PM

NVPTX/implicit-def.ll - this isn't testing what it intended to test, but I don't know how to do that...could just delete the file?

This test is from 2013 and hasn't had a meaningful change since then. It's not totally clear to me what it's testing, but it looks like it's checking for a crasher.

I suspect that in the years since then we're a-ok on covering this. I support deleting it.

(Or if you want to fix up the test like you have it here, that's also OK with me.)

NVPTX/implicit-def.ll - this isn't testing what it intended to test, but I don't know how to do that...could just delete the file?

This test is from 2013 and hasn't had a meaningful change since then. It's not totally clear to me what it's testing, but it looks like it's checking for a crasher.

I suspect that in the years since then we're a-ok on covering this. I support deleting it.

(Or if you want to fix up the test like you have it here, that's also OK with me.)

I think it's better to remove it since there is no implicit def possibility if we fold the undef to NaN. Keeping it around will just confuse others about the intent if it needs changing in the future.

spatel updated this revision to Diff 147499.May 18 2018, 6:59 AM

Patch updated:
Removed NVPTX test since it's no longer testing the implicit def scenario that it was trying to check.

spatel edited the summary of this revision. (Show Details)May 18 2018, 7:00 AM
mcberg2017 accepted this revision.May 18 2018, 2:30 PM

This looks good to me. Up to you Sanjay if you want a second set of eyes to confirm.

This revision is now accepted and ready to land.May 18 2018, 2:30 PM

This looks good to me. Up to you Sanjay if you want a second set of eyes to confirm.

Thanks. I'll let this sit until Monday at least, so others have a chance to reply if they'd like. Let me also add some more potential AMDGPU folks for that test diff in particular.

This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/CodeGen/AMDGPU/mad-mix-lo.ll