[llvm] IR: introduce struct with CmpInst::Predicate and samesign (PR #116867)

[Nikita Popov via llvm-commits]

================
@@ -0,0 +1,40 @@
+//===- CmpPredicate.h - CmpInst Predicate with samesign information -------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// A CmpInst::Predicate with any samesign information (applicable to ICmpInst).
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_IR_CMPPREDICATE_H
+#define LLVM_IR_CMPPREDICATE_H
+
+#include "llvm/IR/InstrTypes.h"
+
+namespace llvm {
+/// An abstraction over a floating-point predicate, and a pack of an integer
+/// predicate with samesign information. The getCmpPredicate() family of
+/// functions in ICmpInst construct and return this type. It is also implictly
+/// constructed with a Predicate, dropping samesign information.
+class CmpPredicate {
+  CmpInst::Predicate Pred;
+  bool HasSameSign;
+
+public:
+  CmpPredicate(CmpInst::Predicate Pred, bool HasSameSign = false)
+      : Pred(Pred), HasSameSign(HasSameSign) {}
+
+  operator CmpInst::Predicate() { return Pred; }
+
+  bool hasSameSign() {
+    assert(CmpInst::isIntPredicate(Pred));
+    return HasSameSign;
+  }
----------------
nikic wrote:

So the reason I'm asking this is code like this: https://github.com/llvm/llvm-project/blob/4a7b56e6e7dd0f83c379ad06b6e81450bc691ba6/llvm/lib/Analysis/ValueTracking.cpp#L9347-L9348

If `LPred == RPred` compares predicate+samesign, then we get an optimization quality regression if we have ult and ult samesign.

If `LPred == RPred` compares predicate without samesign, then we have a potential *correctness* problem, because we e.g. we may have ult samesign and ult, but because they're equal be pass ult samesign to isImpliedCondOperands, and that function then might make use of the samesign information -- even though it's only valid for one of the predicates.

A variant of `LPred == RPred` that also tries to canonicalize signed -> unsigned to match a samesign predicate would have the same problem as the previous point.

So I think the correct thing to do here is actually to `operator==() = delete` and then add a method that does the correct thing, like `std::optional<CmpPredicate> getMatchingPred()` that returns `ult samesign` for `ult samesign + ult samesign`, `ult` for `ult samesign + ult`, `slt` for `ult samesign + slt` and nullopt for `ult + slt`.

If we do want to have `operator==`, I think it should do the conservatively correct (even if likely suboptimal) thing.

https://github.com/llvm/llvm-project/pull/116867