Page MenuHomePhabricator

[SelectionDAG][GISel] Make LegalizeDAG lower FNEG using integer ops.
Needs ReviewPublic

Authored by efriedma on Jul 21 2020, 6:22 PM.

Details

Summary

Previously, if a floating-point type was legal, but FNEG wasn't legal, we would use FSUB. Instead, we should use integer ops, to preserve the semantics. (Alternatively, there's a compiler-rt call we could use, but there isn't much reason to use that.)

It turns out we actually are still using this obscure SelectionDAG codepath in a few cases: on some targets, we have "legal" floating-point types that don't actually support any floating-point operations. In particular, ARM and AArch64 are using this path.

For GlobalISel, all targets without a native FNEG used this codepath.

The implementation for SelectionDAG is pretty simple because we can reuse the infrastructure from FCOPYSIGN. For GlobalISel, it's really easy because we can just blindly emit G_XOR and let legalization of the XOR take care of the rest.

See also 9a3dc3e, the corresponding change to type legalization.

Also includes a "bonus" change to STRICT_FSUB legalization, so we can lower an FSUB_STRICT to a float libcall.

Includes the changes to both LegalizeDAG and GlobalISel so we don't have inconsistent results in the future.

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

Diff Detail

Unit TestsFailed

TimeTest
20 mslinux > Flang.Preprocessing::compiler_defined_macros.F90
Script: -- : 'RUN: at line 6'; /mnt/disks/ssd0/agent/llvm-project/build/bin/f18 -intrinsic-module-directory /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/include/flang -E /mnt/disks/ssd0/agent/llvm-project/flang/test/Preprocessing/compiler_defined_macros.F90 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --ignore-case /mnt/disks/ssd0/agent/llvm-project/flang/test/Preprocessing/compiler_defined_macros.F90

Event Timeline

efriedma created this revision.Jul 21 2020, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 6:22 PM

GlobalISel has the same problem

RKSimon added inline comments.Jul 26 2020, 5:46 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3240

Is it worth moving this to a ExpandFNEG helper like the other ops?

efriedma updated this revision to Diff 284491.Aug 10 2020, 1:27 PM
efriedma edited the summary of this revision. (Show Details)

Refactor. Fix a regression that came up when I rebased.

This also needs to be fixed for globalisel to avoid this bug appearing or not depending on other code in the function hitting the fallback

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1574

The explanation is too vague and not quite correct. It's specifically signaling nans (and also possibly denormal flushing). fsub is a canonicalizing operation, but fneg is not

Do you think the globalisel change has to be done in the same patch? I guess I can try to look into it.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1574

There's also a problem for non-signalling nans: the sign of the result is unspecified. Maybe common implementations flip the sign? Not sure.

Do you think the globalisel change has to be done in the same patch? I guess I can try to look into it.

Yes, but it would also make sense for it to be a separate patch. I just think it’s important that they’re at least consistent in their bugs

efriedma updated this revision to Diff 284530.Aug 10 2020, 5:04 PM
efriedma retitled this revision from [SelectionDAG] Make LegalizeDAG lower FNEG using integer ops. to [SelectionDAG][GISel] Make LegalizeDAG lower FNEG using integer ops..
efriedma edited the summary of this revision. (Show Details)

Updated with corresponding GlobalISel changes.