Page MenuHomePhabricator

[InstSimplify] enhance/fix fcmp fold with never-nan operand

Authored by spatel on Jun 6 2019, 1:45 PM.



This is 1 step towards correcting our usage of fast-math-flags when applied on an fcmp. In this case, we were checking for 'nnan' on the fcmp itself rather than the operand of the fcmp.

By using the more general "isKnownNeverNaN()", we gain a simplification shown on the tests with 'uitofp' regardless of the FMF on the fcmp (uitofp never produces a NaN). On the tests with 'fabs', we are now relying on the FMF for the call fabs instruction rather than the FMF on the fcmp.

If this looks ok, I'll update the 'ult' case below here as a follow-up.

Diff Detail


Event Timeline

spatel created this revision.Jun 6 2019, 1:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2019, 1:45 PM

This LGTM, with one clarification inline...

371 ↗(On Diff #203435)

Why can't we infer that the operand here is >=0 (from the fabs) and not a NaN (from the fcmp's nnan)? Is it not safe to combine that information?

I suppose a poison value could be lost...

spatel marked an inline comment as done.Jun 7 2019, 9:25 AM
spatel added inline comments.
371 ↗(On Diff #203435)

We're trying to move away from the specialized usage of FMF on fcmp because that introduces strangeness like:

More discussion here:

But now that I'm looking at this again, it would be better to split this change into 2 parts:
(1) add the check for isKnownNeverNan()
(2) remove the check for FMF on the fcmp

...because I'm not sure if we will introduce regressions by not checking for FMF on fcmp. We need to get further along with propagating/using FMF on select and possibly other instructions before we tighten up the fcmp semantics.

spatel updated this revision to Diff 203574.Jun 7 2019, 9:39 AM

Patch updated:
Only add a check for isKnownNeverNaN(). We can remove FMF.noNaNs() independently when that's less likely to introduce regressions.

cameron.mcinally accepted this revision.Jun 7 2019, 12:28 PM

Good compromise. LGTM.

371 ↗(On Diff #203435)

I'll read through the links this weekend, but no reason to delay your updated patch...

This revision is now accepted and ready to land.Jun 7 2019, 12:28 PM
This revision was automatically updated to reflect the committed changes.