[llvm] Fix fabs simplification

[ojhunt]

The existing code hits the std::optional is of comparing to a bool being "correct" for the std::optional and the intended comparison.

This corrects the comparison while reinforcing the idea that we need some way to diagnose this.

Fixes #152824

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Oliver Hunt (ojhunt)

Changes

The existing code hits the std::optional<bool> is of comparing to a bool being "correct" for the std::optional and the intended comparison.

This corrects the comparison while reinforcing the idea that we need some way to diagnose this.

Fixes #152824

---

Full diff: https://github.com/llvm/llvm-project/pull/152825.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/InstructionSimplify.cpp (+1-1)

diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index 5907e21065331..afe6275d0426e 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -6304,7 +6304,7 @@ static Value *simplifyUnaryIntrinsic(Function *F, Value *Op0,
   Value *X;
   switch (IID) {
   case Intrinsic::fabs:
-    if (computeKnownFPSignBit(Op0, Q) == false)
+    if (*computeKnownFPSignBit(Op0, Q) == false)
       return Op0;
     break;
   case Intrinsic::bswap:

[ojhunt]

I can make a test for this at the clang level but this seems like it should be an llvm/ir test and I have no idea how to write it :D

[ojhunt]

I realized it's possible I've just completely broken fabs-optimizations so I need to run more than clang tests :D

[ojhunt]

Ok, this is weird, I believe (based on other code) that this is the "correct" fix for this specific piece of code, but making that fix does not actually fix the original bug?

[efriedma-quic]

Given std::optional<bool>Res;, Res == false, Res == std::optional<bool>(false), and Res && *Res == false all mean the same thing.

[ojhunt]

Given std::optional<bool>Res;, Res == false, Res == std::optional<bool>(false), and Res && *Res == false all mean the same thing.

Is that the conversion path that is actually happening? (and yes with more testing I found the actual issue is that in my first pass I was just disabling the optimization) I'm closing this PR for now, although independent of fixing anything I think this code would be clearer with the * + == version.

[efriedma-quic]

operator== is overloaded, so it's not literally doing the conversion, but yes.