Page MenuHomePhabricator

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

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

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
3274

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
1579

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
1579

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.

@arsenm Any more thoughts?

llvm/lib/Target/ARM/ARMLegalizerInfo.cpp
91

should this be a separate fix?

llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir
2

Worth adding a f80 case as well?

arsenm accepted this revision.Wed, Sep 23, 5:42 AM
This revision is now accepted and ready to land.Wed, Sep 23, 5:42 AM
efriedma added inline comments.Wed, Sep 23, 12:12 PM
llvm/lib/Target/ARM/ARMLegalizerInfo.cpp
91

The tests in this patch exercise this codepath. I guess it could also be tested independently; I'll look at adding a few more tests.

llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir
2

f80 currently isn't supported by GlobalISel.

RKSimon added inline comments.Wed, Sep 23, 1:18 PM
llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir
2

ok - I was mainly interested in seeing what happened with a non-legal integer type - not a big issue though

arsenm added inline comments.Wed, Sep 23, 1:53 PM
llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir
2

It doesn't work end to end, but it should work here if you use an 80 bit value

efriedma added inline comments.Wed, Sep 23, 2:08 PM
llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir
2

I wrote a test, and got "LLVM ERROR: unable to legalize instruction: %1:_(s80) = G_XOR %0:_, %2:_ (in function: test_fneg_f80)".

This revision was landed with ongoing or failed builds.Wed, Sep 23, 2:11 PM
This revision was automatically updated to reflect the committed changes.