[PATCH] D123830: [InstCombine] Complete folding of fneg-of-fabs

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D123830#3466496 <https://reviews.llvm.org/D123830#3466496>, @Chenbing.Zheng wrote:

> Thanks for spatel's suggestion , that's very useful.

Yes, this looks nicer. See inline for some small improvements.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2503
+static Value *foldSelectToFabs(SelectInst &SI, InstCombiner::BuilderTy &Builder,
+                               bool &IsNeg, int Swap) {
+  Value *CondVal = SI.getCondition();
----------------
Swap should be a `bool` parameter, and the caller can use "for (bool Swap : {false, true} )".
Or we can move the FNeg part of the transform into this function and avoid the parameter completely?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2506
+  Value *TrueVal = Swap ? SI.getFalseValue() : SI.getTrueValue();
+  Value *FalseVal = Swap ? SI.getTrueValue() : SI.getFalseValue();
+  CmpInst::Predicate Pred;
----------------
If we name this value 'X', then the code would match the comments, so that seems easier to read - see the next comments for a possible reduction.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2512-2513
+
+  // 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.
+  // (X <= +/-0.0) ? (0.0 - X) : X --> fabs(X)
----------------
We can move this statement to above the function declaration now. It describes the complete behavior of this function.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2532-2533
+                  Pred == FCmpInst::FCMP_UGT || Pred == FCmpInst::FCMP_UGE;
+  if (match(TrueVal, m_FNeg(m_Specific(FalseVal))) && SI.hasNoSignedZeros()) {
+    if ((Swap == 0 && IsLTOrLE) || (Swap == 1 && IsGTOrGE))
+      return Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
----------------
This logic seems more complicated than needed. If we always match the TrueVal as -X, then we can get the swapped predicate and reduce the code to something like this:

```
  if (!match(TrueVal, m_FNeg(m_Specific(FalseVal))) || !SI.hasNoSignedZeros())
    return nullptr;

  if (Swap)
    Pred = FCmpInst::getSwappedPredicate(Pred);

  // With nsz:
  // fold (X <  +/-0.0) ? -X : X or (X <= +/-0.0) ? -X : X to fabs(X)
  // fold (X >  +/-0.0) ? -X : X or (X >= +/-0.0) ? X : -X to -fabs(x)
  bool IsLTOrLE = Pred == FCmpInst::FCMP_OLT || Pred == FCmpInst::FCMP_OLE ||
                  Pred == FCmpInst::FCMP_ULT || Pred == FCmpInst::FCMP_ULE;
  bool IsGTOrGE = Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_OGE ||
                  Pred == FCmpInst::FCMP_UGT || Pred == FCmpInst::FCMP_UGE;

  if (IsLTOrLE)
    return Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
  if (IsGTOrGE) {
    IsNeg = true;
    return Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
  }

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123830