Page MenuHomePhabricator

[ValueTracking] determine sign of 0.0 from select when matching min/max FP
ClosedPublic

Authored by spatel on Nov 1 2018, 2:48 PM.

Details

Summary

In PR39475:
https://bugs.llvm.org/show_bug.cgi?id=39475
..we may fail to recognize/simplify fabs() in some cases because we do not canonicalize fcmp with a -0.0 operand.

Adding that canonicalization can cause regressions on min/max FP tests, so that's this patch: for the purpose of determining whether something is min/max, let the value returned by the select determine how we treat a 0.0 operand in the fcmp.

This patch doesn't actually change the -0.0 to +0.0. It just changes the analysis, so we don't fail to recognize equivalent min/max patterns that only differ in the signbit of 0.0.

Diff Detail

Event Timeline

spatel created this revision.Nov 1 2018, 2:48 PM
arsenm added inline comments.Nov 1 2018, 3:20 PM
lib/Analysis/ValueTracking.cpp
4762–4773

This looks big. Can you avoid repeating the same thing for the true and false cases?

spatel added inline comments.Nov 2 2018, 7:49 AM
lib/Analysis/ValueTracking.cpp
4762–4773

Yes, it's a bit ugly. There really are 8 variations to test for from I see, but we can do better by matching with m_AnyZeroFP.
Unfortunately, I left out a potential vector mishap with undefs here, so I need to check for that.

spatel updated this revision to Diff 172356.Nov 2 2018, 7:54 AM

Patch updated:

  1. Reduced code by checking for any FP zero followed by mismatch.
  2. Added checks and unit tests to ignore vector constants with undef elements.
arsenm accepted this revision.Nov 2 2018, 12:30 PM

LGTM

This revision is now accepted and ready to land.Nov 2 2018, 12:30 PM
This revision was automatically updated to reflect the committed changes.