[InstCombine] Remove nnan requirement for transformation to fabs from select

[Krishnakariya]

Summary

In this patch, the "nnan" requirement is removed for the canonicalization of select with fcmp to fabs.
(i) FSub logic: Remove check for nnan flag presence in fsub. Example: https://alive2.llvm.org/ce/z/751svg (fsub).
(ii) FNeg logic: Remove check for the presence of nnan and nsz flag in fneg. Example: https://alive2.llvm.org/ce/z/a_fsdp (fneg).

[lebedev.ri]

It would be good to mention in the description *why* this is being done, why this is okay (alive2 link would suffice)

[Krishnakariya]

In D106872#2907220, @lebedev.ri wrote:

It would be good to mention in the description *why* this is being done, why this is okay (alive2 link would suffice)

Sure, I have updated the description.

[spatel]

This was discussed as a follow-up for D101727.
I originally added the nnan check to these transforms to be extra safe (and so watch for potential fallout on out-of-tree targets), but it's not necessary in the default LLVM FP environment to preserve exact bits of a nnan.
Ie, the sign of nnan will always be cleared by fabs, but not in the original code.

I'm not sure what the tests are showing. I would expect there to be 4 tests corresponding to each code change with no nnan that are now getting transformed?
Please pre-commit all tests with baseline CHECKs as a preliminary patch (no review is needed for that).
If you do not have commit access, now is a good time to request it. :)
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[Krishnakariya]

Added new tests

[spatel]

In D106872#2918265, @Krishnakariya wrote:

Added new tests

That looks fine - please commit the tests to main, so we'll just have test *diffs* in this patch.
You do not need to open a new Phab review for the tests only.
Just push an NFC patch to main once you get commit access.

[Krishnakariya]

Updated diff after pre-committing tests.

[Krishnakariya]

I committed this patch by mistake. I am extremely sorry about this.
I've reverted the commit.

[lebedev.ri]

In D106872#2922101, @Krishnakariya wrote:

I committed this patch by mistake. I am extremely sorry about this.
I've reverted the commit.

FWIW reading through the disscussion i would think this has been basically accepted, if not stamped yet for formal reasons of wanting the tests to be precommitted.

[Krishnakariya]

In D106872#2922157, @lebedev.ri wrote:

In D106872#2922101, @Krishnakariya wrote:

I committed this patch by mistake. I am extremely sorry about this.
I've reverted the commit.

FWIW reading through the disscussion i would think this has been basically accepted, if not stamped yet for formal reasons of wanting the tests to be precommitted.

I have pre-committed the tests and updated the diff. Commit link: https://reviews.llvm.org/rG56e7b6c3924d7ba8db70c38235a77ed8208795eb .

[spatel]

I was out for a few days, so I missed the last few comments. IIUC, the patch was reverted, so I'll re-open the review, mark as accepted, and then you can commit cleanly.

[spatel]

LGTM

[Krishnakariya]

Updating the same diff to commit this patch.

[nick]

llvm/test/Transforms/InstCombine/fabs.ll

252–253

The comment had not been updated and is wrong now.

[Krishnakariya]

llvm/test/Transforms/InstCombine/fabs.ll

252–253

Ohh, thanks for finding it out! I am removing this comment.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

if ((Cmp0 == TrueVal && Cmp1 == FalseVal) ||
         return replaceInstUsesWith(SI, NewSel);
       }
 
       // NOTE: if we wanted to, this is where to detect MIN/MAX
     }
   }
 
   // Canonicalize select with fcmp to fabs(). -0.0 makes this tricky. We need
-  // fast-math-flags (nsz) or fsub with +0.0 (not fneg) for this to work. We
-  // also require nnan because we do not want to unintentionally change the
-  // sign of a NaN value.
+  // fast-math-flags (nsz) or fsub with +0.0 (not fneg) for this to work.
   // (X <= +/-0.0) ? (0.0 - X) : X --> fabs(X)
   Instruction *FSub;
   if (match(CondVal, m_FCmp(Pred, m_Specific(FalseVal), m_AnyZeroFP())) &&
       match(TrueVal, m_FSub(m_PosZeroFP(), m_Specific(FalseVal))) &&
-      match(TrueVal, m_Instruction(FSub)) && FSub->hasNoNaNs() &&
+      match(TrueVal, m_Instruction(FSub)) &&
       (Pred == FCmpInst::FCMP_OLE || Pred == FCmpInst::FCMP_ULE)) {
     Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
     return replaceInstUsesWith(SI, Fabs);
   }
   // (X >  +/-0.0) ? X : (0.0 - X) --> fabs(X)
   if (match(CondVal, m_FCmp(Pred, m_Specific(TrueVal), m_AnyZeroFP())) &&
       match(FalseVal, m_FSub(m_PosZeroFP(), m_Specific(TrueVal))) &&
-      match(FalseVal, m_Instruction(FSub)) && FSub->hasNoNaNs() &&
+      match(FalseVal, m_Instruction(FSub)) &&
       (Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_UGT)) {
     Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, TrueVal, &SI);
     return replaceInstUsesWith(SI, Fabs);
   }
   // With nnan and nsz:
   // (X <  +/-0.0) ? -X : X --> fabs(X)
   // (X <= +/-0.0) ? -X : X --> fabs(X)
   Instruction *FNeg;
   if (match(CondVal, m_FCmp(Pred, m_Specific(FalseVal), m_AnyZeroFP())) &&
       match(TrueVal, m_FNeg(m_Specific(FalseVal))) &&
-      match(TrueVal, m_Instruction(FNeg)) && FNeg->hasNoNaNs() &&
-      FNeg->hasNoSignedZeros() && SI.hasNoSignedZeros() &&
+      match(TrueVal, m_Instruction(FNeg)) && SI.hasNoSignedZeros() &&
       (Pred == FCmpInst::FCMP_OLT || Pred == FCmpInst::FCMP_OLE ||
        Pred == FCmpInst::FCMP_ULT || Pred == FCmpInst::FCMP_ULE)) {
     Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
     return replaceInstUsesWith(SI, Fabs);
   }
   // With nnan and nsz:
   // (X >  +/-0.0) ? X : -X --> fabs(X)
   // (X >= +/-0.0) ? X : -X --> fabs(X)
   if (match(CondVal, m_FCmp(Pred, m_Specific(TrueVal), m_AnyZeroFP())) &&
       match(FalseVal, m_FNeg(m_Specific(TrueVal))) &&
-      match(FalseVal, m_Instruction(FNeg)) && FNeg->hasNoNaNs() &&
-      FNeg->hasNoSignedZeros() && SI.hasNoSignedZeros() &&
+      match(FalseVal, m_Instruction(FNeg)) && SI.hasNoSignedZeros() &&
       (Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_OGE ||
        Pred == FCmpInst::FCMP_UGT || Pred == FCmpInst::FCMP_UGE)) {
     Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, TrueVal, &SI);
     return replaceInstUsesWith(SI, Fabs);
   }
 
   // See if we are selecting two values based on a comparison of the two values.
   if (ICmpInst *ICI = dyn_cast<ICmpInst>(CondVal))

llvm/test/Transforms/InstCombine/fabs.ll

 ; CHECK-NEXT:    ret double [[FABS]]
 ;
   %fpext = fpext float %x to double
   %fabs = call double @llvm.fabs.f64(double %fpext)
   store volatile double %fpext, double* undef
   ret double %fabs
 }
 
 ; Negative test for the fabs folds below: we require nnan, so
 ; we won't always clear the sign bit of a NaN value.
 
 define double @select_fcmp_ole_zero(double %x) {
 ; CHECK-LABEL: @select_fcmp_ole_zero(
-; CHECK-NEXT:    [[LEZERO:%.*]] = fcmp ole double [[X:%.*]], 0.000000e+00
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub double 0.000000e+00, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[LEZERO]], double [[NEGX]], double [[X]]
-; CHECK-NEXT:    ret double [[FABS]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fabs.f64(double [[X:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %lezero = fcmp ole double %x, 0.0
   %negx = fsub double 0.0, %x
   %fabs = select i1 %lezero, double %negx, double %x
   ret double %fabs
 }
 
 ; X <= 0.0 ? (0.0 - X) : X --> fabs(X)
;
   %fabs = select nnan <2 x i1> %lezero, <2 x float> %negx, <2 x float> %x
   ret <2 x float> %fabs
 }
 
 ; X > 0.0 ? X : (0.0 - X) --> fabs(X)
 
 define fp128 @select_fcmp_ogt_zero(fp128 %x) {
 ; CHECK-LABEL: @select_fcmp_ogt_zero(
-; CHECK-NEXT:    [[GTZERO:%.*]] = fcmp ogt fp128 [[X:%.*]], 0xL00000000000000000000000000000000
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub fp128 0xL00000000000000000000000000000000, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[GTZERO]], fp128 [[X]], fp128 [[NEGX]]
-; CHECK-NEXT:    ret fp128 [[FABS]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call fp128 @llvm.fabs.f128(fp128 [[X:%.*]])
+; CHECK-NEXT:    ret fp128 [[TMP1]]
 ;
   %gtzero = fcmp ogt fp128 %x, zeroinitializer
   %negx = fsub fp128 zeroinitializer, %x
   %fabs = select i1 %gtzero, fp128 %x, fp128 %negx
   ret fp128 %fabs
 }
 
 define fp128 @select_fcmp_nnan_ogt_zero(fp128 %x) {
;
   %fabs = select i1 %gtzero, half %x, half %negx
   ret half %fabs
 }
 
 ; X < 0.0 ? -X : X --> fabs(X)
 
 define double @select_fcmp_olt_zero_unary_fneg(double %x) {
 ; CHECK-LABEL: @select_fcmp_olt_zero_unary_fneg(
-; CHECK-NEXT:    [[LTZERO:%.*]] = fcmp olt double [[X:%.*]], 0.000000e+00
-; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select nsz i1 [[LTZERO]], double [[NEGX]], double [[X]]
-; CHECK-NEXT:    ret double [[FABS]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %ltzero = fcmp olt double %x, 0.0
   %negx = fneg double %x
   %fabs = select nsz i1 %ltzero, double %negx, double %x
   ret double %fabs
 }
 
 define double @select_fcmp_nnan_nsz_olt_zero(double %x) {
;
   %fabs = select i1 %lezero, float %negx, float %x
   ret float %fabs
 }
 
 ; X > 0.0 ? X : (0.0 - X) --> fabs(X)
 
 define <2 x float> @select_fcmp_ogt_zero_unary_fneg(<2 x float> %x) {
 ; CHECK-LABEL: @select_fcmp_ogt_zero_unary_fneg(
-; CHECK-NEXT:    [[GTZERO:%.*]] = fcmp ogt <2 x float> [[X:%.*]], zeroinitializer
-; CHECK-NEXT:    [[NEGX:%.*]] = fneg <2 x float> [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select nsz <2 x i1> [[GTZERO]], <2 x float> [[X]], <2 x float> [[NEGX]]
-; CHECK-NEXT:    ret <2 x float> [[FABS]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz <2 x float> @llvm.fabs.v2f32(<2 x float> [[X:%.*]])
+; CHECK-NEXT:    ret <2 x float> [[TMP1]]
 ;
   %gtzero = fcmp ogt <2 x float> %x, zeroinitializer
   %negx = fneg <2 x float> %x
   %fabs = select nsz <2 x i1> %gtzero, <2 x float> %x, <2 x float> %negx
   ret <2 x float> %fabs
 }
 
 define <2 x float> @select_fcmp_nnan_nsz_ogt_zero(<2 x float> %x) {