Page MenuHomePhabricator

[InstCombine] propagate fast-math-flags (FMF) to select when inverting fcmp+select
ClosedPublic

Authored by spatel on Nov 1 2019, 10:00 AM.

Details

Summary

As noted by the FIXME comment, this is not correct based on our current FMF semantics. We should be propagating FMF from the final value in a sequence (in this case the 'select'). So the behavior even without this patch is wrong, but we did not allow FMF on 'select' until recently.

But if we do the correct thing right now in this patch, we'll inevitably introduce regressions because we have not wired up FMF propagation for 'phi' and 'select' in other passes (like SimplifyCFG) or other places in InstCombine. I'm not seeing a better incremental way to make progress.

That said, the potential extra damage over the existing wrong behavior from this patch is very limited. AFAIK, the only way to have different FMF on IR in the same function is if we have LTO inlined IR from 2 modules that were compiled using different fast-math settings.

As seen in the tests, we may actually see some improvements with this patch because adding the FMF to the 'select' allows matching to min/max intrinsics that were previously missed (in the common case, the 'fcmp' and 'select' should have identical FMF to begin with).

Next steps in the transition:

  1. Make similar changes in instcombine as needed.
  2. Enable phi-to-select FMF propagation in SimplifyCFG.
  3. Remove dependencies on fcmp with FMF.
  4. Deprecate FMF on fcmp.

Diff Detail

Event Timeline

spatel created this revision.Nov 1 2019, 10:00 AM
spatel edited the summary of this revision. (Show Details)Nov 1 2019, 10:02 AM
spatel edited the summary of this revision. (Show Details)
mcberg2017 accepted this revision.Nov 7 2019, 4:51 PM

It is a step in the right direction. I tried it out internally and its fine for us. We too have work to do wrt phi mapping in GiSel wrt flags. So initially, LGTM. You might want to see what others say and so leave it for bit...

This revision is now accepted and ready to land.Nov 7 2019, 4:51 PM
This revision was automatically updated to reflect the committed changes.