This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Add nsz constraint to aggressive fma folding
ClosedPublic

Authored by qiucf on Mar 19 2020, 2:13 AM.

Details

Summary

There's a folding pattern in DAGCombine:

(fsub x, (fma y, z, (fmul u, v)))
->
(fma (fneg y), z, (fma (fneg u), v, x))

However, since -0-0=-0; -0+0=+0, if yz=1, uv=-1, x=-0:

x-(yz+uv) = -0
-yz+(-uv+x) = +0

So this requires nsz.

Diff Detail

Event Timeline

qiucf created this revision.Mar 19 2020, 2:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2020, 2:13 AM
spatel added inline comments.Mar 19 2020, 4:48 AM
llvm/test/CodeGen/AMDGPU/fma-combine.ll
3

It would be better to fix this RUN line (or better: add IR-level FMF to the changed test).

llvm/test/CodeGen/AMDGPU/mad-combine.ll
5

It would be better to fix this RUN line (or better: add IR-level FMF to the changed test).

llvm/test/CodeGen/PowerPC/fma-assoc.ll
133–136

Please create a test that uses IR-level FMF ("reassoc nsz" or "fast"?) entirely. If you can update this file to avoid the global "-enable-unsafe-fp-math" as a preliminary commit, that would be best.

It would also be great to use "utils/update_llc_test_checks.py" to auto-generate complete CHECK lines as a preliminary NFC commit.

qiucf updated this revision to Diff 251565.Mar 20 2020, 12:47 AM
qiucf marked 3 inline comments as done.

Update some test cases

spatel accepted this revision.Mar 20 2020, 9:42 AM

LGTM

llvm/test/CodeGen/PowerPC/fma-assoc.ll
133–136

That's not what I was hoping for, but I see now that this whole test file should be updated. We should treat "-enable-unsafe-fp-math" on the RUN line as deprecated and replace that with "fast" or "reassoc" in the IR. But that will require duplicating tests if I'm seeing it correctly.

I won't block this correctness fix on the test improvements, but please make that change as an NFC follow-up or least mark this file with a FIXME.

This revision is now accepted and ready to land.Mar 20 2020, 9:42 AM
qiucf marked an inline comment as done.Mar 22 2020, 8:18 AM
qiucf added inline comments.
llvm/test/CodeGen/PowerPC/fma-assoc.ll
133–136

Thanks for explanation. I committed rGc1bc56bf4f and rG996dc13dc first and then this one. BTW, options/flags for FP operations may sometimes be confusing. It seems they should be clarified/reorganized.

This revision was automatically updated to reflect the committed changes.
spatel added inline comments.Mar 22 2020, 8:54 AM
llvm/test/CodeGen/PowerPC/fma-assoc.ll
133–136

Thanks for cleaning that test file! If you have a suggestion for improving the docs/comments for these flags, then we can fix it. I agree that it is messy.