Page MenuHomePhabricator

[InstCombine] Infer fast math flags on fadd/fsub/fmul/fcmp
Needs ReviewPublic

Authored by bkramer on Nov 20 2019, 5:47 AM.

Details

Summary

Applies nnan, ninf and nsz. This allows more instructions to be folded
away even with no fast math flags, e.g. (int)x * (b ? 1.0 : 0.0) -> b ? x : 0.0

As a side effect this will propagate fast math flags out of inlined fast
math code into surrounding functions.

Event Timeline

bkramer created this revision.Nov 20 2019, 5:47 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 20 2019, 5:47 AM
bkramer updated this revision to Diff 230245.Nov 20 2019, 5:48 AM

Fix condition

I like the idea, but I'd be more comfortable reviewing the diffs in stages, so we know that the test coverage for the value tracking calls is good. So I'd prefer if we split this somehow - either by the opcode callers (fadd, fsub, fmul...) or the the FMF analysis (nnan, nsz, ninf). That raises a few questions:

  1. Why aren't fdiv and frem included?
  2. Can we infer FMF for FP intrinsics/libcalls/select/phi? (follow-on patches)
  3. We're moving away from FMF on fcmp (recent step: rGebf9bf2cbc8f), so is it worth including starting from fcmp, or can we wait for that part to settle? (Side question may be if/when we're going to allow FMF on fptrunc/fpextend).

For us this would be an impediment as we have math models that want ieee behavior while relaxing precision. Adding nnan or ninf would obstruct those choices.

For us this would be an impediment as we have math models that want ieee behavior while relaxing precision. Adding nnan or ninf would obstruct those choices.

Mind elaborating why nnan/ninf are problematic for you? They're supposed to be a hint to the optimizer and can be dropped any time.

I like the idea, but I'd be more comfortable reviewing the diffs in stages, so we know that the test coverage for the value tracking calls is good. So I'd prefer if we split this somehow - either by the opcode callers (fadd, fsub, fmul...) or the the FMF analysis (nnan, nsz, ninf). That raises a few questions:

  1. Why aren't fdiv and frem included?

We currently cannot infer anything for fdiv/frem in isKnownNeverNaN/Inf so there's no way to test it.

  1. Can we infer FMF for FP intrinsics/libcalls/select/phi? (follow-on patches)

Yeah, that's a logical followup

  1. We're moving away from FMF on fcmp (recent step: rGebf9bf2cbc8f), so is it worth including starting from fcmp, or can we wait for that part to settle? (Side question may be if/when we're going to allow FMF on fptrunc/fpextend).

I'll drop fcmp then and split this up once we know that it's actually a direction we want to pursue.