[PATCH] D101727: Fix PR47960 - Incorrect transformation of fabs with nnan flag

Fri Jul 23 08:36:27 PDT 2021

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - I think this is safe now and seems to fix the bugs noted by Alive2 , but we really should follow-up by cleaning this up: we don't need FMF at all in some cases, and we don't need to worry about preserving sign of NaN in the default FP environment.

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2873
       match(TrueVal, m_FSub(m_PosZeroFP(), m_Specific(FalseVal))) &&
       match(TrueVal, m_Instruction(FSub)) && FSub->hasNoNaNs() &&
       (Pred == FCmpInst::FCMP_OLE || Pred == FCmpInst::FCMP_ULE)) {
----------------
We could ease this condition, right?
If either of the fsub or the select has nnan, then the result can have nnan. It seems we don't need any FMF at all on some of these patterns:
https://alive2.llvm.org/ce/z/3bfTSb

Let's not complicate this patch any more though. If that's correct, then let's make changes as a follow-up patch (with more tests to confirm).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101727/new/

https://reviews.llvm.org/D101727