This is an archive of the discontinued LLVM Phabricator instance.

[instcombine] remove fsub to fneg hacks; only emit fneg
ClosedPublic

Authored by simoll on Mar 2 2020, 9:02 AM.

Details

Summary

Rewrite the fsub-0.0 idiom to fneg and always emit fneg for fp negation. This also extends the scalarization cost in instcombine for unary operators to result in the same IR rewrites for fneg as for the idiom.

Diff Detail

Event Timeline

simoll created this revision.Mar 2 2020, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 9:02 AM
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2137

Just noticed that this is a bug -- it needs a guard for denormal flushing.

When FTZ or DAZ:

fsub -0.0, Denorm ==> +-0
fneg Denorm ==> -Denorm

Handling denormals correctly is an ongoing project. If you'd like to leave this for later, we'll get to it. I see that it's currently broken, so no harm in continuing that way for now.

Could you add a brief FIXME here?

simoll updated this revision to Diff 247820.Mar 3 2020, 1:27 AM
simoll marked an inline comment as done.

Thanks for the review!

  • rebased
  • fixed clang test
  • added FIXME for correct DTZ, DAZ handling
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2020, 1:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
simoll marked an inline comment as done.Mar 3 2020, 1:30 AM
simoll added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2137

Added a FIXME for now.
I suppose this should be fixed in PatternMatch.h by making m_FNeg refuse to match the fsub idiom for FTZ/DAZ if fneg does not preserve denormal semantics under those settings.

simoll updated this revision to Diff 247850.Mar 3 2020, 4:09 AM

NFC

  • rebased
  • formatting

ping. Is this good to go?
(merge bot failures are due to the m_UnOp syntax, which goes against the coding standard).

cameron.mcinally accepted this revision.Mar 10 2020, 7:21 AM

LGTM. Thanks, Simon.

This revision is now accepted and ready to land.Mar 10 2020, 7:21 AM

Sorry, forgot to submit my nit...

llvm/include/llvm/IR/PatternMatch.h
75

Nit: Comment needs updating.

spatel added inline comments.Mar 10 2020, 7:25 AM
llvm/include/llvm/IR/PatternMatch.h
622–624

Match a binary -> Match a unary

802–803

The copied comment doesn't make sense for a unary op; there's no commutability/LHS here.

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
379

typo: cmatchers?

simoll updated this revision to Diff 249396.Mar 10 2020, 8:39 AM
simoll marked 5 inline comments as done.
  • NFC
  • Fixed code comments
  • Rebased
spatel accepted this revision.Mar 10 2020, 9:11 AM

LGTM - see inline for 1 more comment nit.

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2132–2133

This comment is stale. Remove or update to something like:

// Convert fsub from zero to the canonical fneg instruction.
This revision was automatically updated to reflect the committed changes.