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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D101727#2746020 <https://reviews.llvm.org/D101727#2746020>, @nlopes wrote:

> FYI just added support for select w/ fast-math to Alive2 so we can check these tests.

Great! Is it available on the online instance yet?
Checking my understanding on this example:
https://alive2.llvm.org/ce/z/M2Fe49

  define double @src(double %x) {
  %0:
    %lezero = fcmp ole double %x, 0.000000
    %negx = fsub nnan double 0.000000, %x
    %fabs = select i1 %lezero, double %negx, double %x
    ret double %fabs
  }
  =>
  define double @tgt(double %x) {
  %0:
    %fabs = fabs nnan double %x
    ret double %fabs
  }
  Transformation doesn't verify!
  
  ERROR: Target is more poisonous than source
  
  Example:
  double %x = NaN
  
  Source:
  i1 %lezero = #x0 (0)
  double %negx = poison
  double %fabs = NaN
  
  Target:
  double %fabs = poison
  Source value: NaN
  Target value: poison

In the source function, if we have "nnan" on the select, and we choose %x (NaN), then the return value must be poison? The "nnan" on the select was dropped in the output pane though?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2863
       (Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_UGT)) {
     Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, TrueVal, FSub);
     return replaceInstUsesWith(SI, Fabs);
----------------
What about this one and the fneg transforms below?
If the FMF propagation logic is identical, we should change all of these simultaneously to keep things consistent.
We already have regression tests for all 4 variants (but you might want to add extra tests to make sure we're not losing coverage).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101727