[KnownBits] Remove `hasConflict()` assertions #94568

c8ef · 2024-06-06T04:37:55Z

close: #94436

llvmbot · 2024-06-06T04:38:24Z

@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-support

Author: None (c8ef)

Changes

close: #94436

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

1 Files Affected:

(modified) llvm/include/llvm/Support/KnownBits.h (+2-9)

diff --git a/llvm/include/llvm/Support/KnownBits.h b/llvm/include/llvm/Support/KnownBits.h
index ba4a5f01036ca..c206bd77d5880 100644
--- a/llvm/include/llvm/Support/KnownBits.h
+++ b/llvm/include/llvm/Support/KnownBits.h
@@ -48,7 +48,6 @@ struct KnownBits {
 
   /// Returns true if we know the value of all bits.
   bool isConstant() const {
-    assert(!hasConflict() && "KnownBits conflict!");
     return Zero.popcount() + One.popcount() == getBitWidth();
   }
 
@@ -74,16 +73,10 @@ struct KnownBits {
   }
 
   /// Returns true if value is all zero.
-  bool isZero() const {
-    assert(!hasConflict() && "KnownBits conflict!");
-    return Zero.isAllOnes();
-  }
+  bool isZero() const { return Zero.isAllOnes(); }
 
   /// Returns true if value is all one bits.
-  bool isAllOnes() const {
-    assert(!hasConflict() && "KnownBits conflict!");
-    return One.isAllOnes();
-  }
+  bool isAllOnes() const { return One.isAllOnes(); }
 
   /// Make all bits known to be zero and discard any previous information.
   void setAllZero() {

nikic · 2024-06-06T06:30:13Z

These assertions are not just in KnownBits.h, but also spread across many other places. Basically do a git grep "hasConflict()" (There are currently 88 occurrences, though not all of them are in assertions.)

llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp

c8ef · 2024-06-06T08:15:51Z

These assertions are not just in KnownBits.h, but also spread across many other places. Basically do a git grep "hasConflict()" (There are currently 88 occurrences, though not all of them are in assertions.)

I removed more assertions in other files.

llvm/lib/Analysis/ValueTracking.cpp

llvm/lib/IR/ConstantRange.cpp

nikic · 2024-06-06T10:34:48Z

There are more checks in KnownBitsTest, but I guess we can drop those in a second step, when removing the various if (!hasConflict()) setAllZero(); bits.

nikic · 2024-06-06T10:36:10Z

Though one testing-related change we should do is drop this check:

llvm-project/llvm/unittests/Support/KnownBitsTest.h

Line 29 in 574ab7e

if (Known.hasConflict())

This means the KnownBits implementation will actually get tested with conflict values, so we can be sure this doesn't produce unexpected crashes somewhere.

c8ef · 2024-06-06T11:08:49Z

Though one testing-related change we should do is drop this check:

llvm-project/llvm/unittests/Support/KnownBitsTest.h

Line 29 in 574ab7e

if (Known.hasConflict())

This means the KnownBits implementation will actually get tested with conflict values, so we can be sure this doesn't produce unexpected crashes somewhere.

After removing this statement, I immediately encounter an error locally. Is this the expected behavior? Since the purpose of this PR is to enable a bit to be both zero and one simultaneously, perhaps we should also remove this assert?

[==========] Running 16 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 16 tests from KnownBitsTest
[ RUN      ] KnownBitsTest.AddCarryExhaustive
Assertion failed: !(CarryZero && CarryOne) && "Carry can't be zero and one at the same time", file .\llvm-project\llvm\lib\Support\KnownBits.cpp, line 25

UPD:

If this assertion is also removed, there will be numerous failures such as:

llvm-project\llvm\unittests\Support\KnownBitsTest.cpp(138): error: Expected equality of these values:
  Exact
    Which is: !!!!
  Computed
    Which is: ????

jayfoad · 2024-06-06T11:27:39Z

I expect you would have to make some changes like this, since even the supposedly optimal KnownBits implementations do not always report conflict when they should:

diff --git a/llvm/unittests/Support/KnownBitsTest.cpp b/llvm/unittests/Support/KnownBitsTest.cpp
index 824cf7501fd4..acec5dbfb4e6 100644
--- a/llvm/unittests/Support/KnownBitsTest.cpp
+++ b/llvm/unittests/Support/KnownBitsTest.cpp
@@ -95,9 +95,10 @@ static void testBinaryOpExhaustive(StringRef Name, BinaryBitsFn BitsFn,
           });
         });
 
-        EXPECT_TRUE(!Computed.hasConflict());
-        EXPECT_TRUE(checkResult(Name, Exact, Computed, {Known1, Known2},
-                                CheckOptimality));
+        if (!Exact.hasConflict()) {
+          EXPECT_TRUE(checkResult(Name, Exact, Computed, {Known1, Known2},
+                                  CheckOptimality));
+        }
         // In some cases we choose to return zero if the result is always
         // poison.
         if (RefinePoisonToZero && Exact.hasConflict()) {

c8ef · 2024-06-06T11:41:02Z

static KnownBits computeForAddCarry(
    const KnownBits &LHS, const KnownBits &RHS,
    bool CarryZero, bool CarryOne) {
  APInt PossibleSumZero = LHS.getMaxValue() + RHS.getMaxValue() + !CarryZero;
  APInt PossibleSumOne = LHS.getMinValue() + RHS.getMinValue() + CarryOne;

  // Compute known bits of the carry.
  APInt CarryKnownZero = ~(PossibleSumZero ^ LHS.Zero ^ RHS.Zero);
  APInt CarryKnownOne = PossibleSumOne ^ LHS.One ^ RHS.One;

  // Compute set of known bits (where all three relevant bits are known).
  APInt LHSKnownUnion = LHS.Zero | LHS.One;
  APInt RHSKnownUnion = RHS.Zero | RHS.One;
  APInt CarryKnownUnion = std::move(CarryKnownZero) | CarryKnownOne;
  APInt Known = std::move(LHSKnownUnion) & RHSKnownUnion & CarryKnownUnion;

  // vvvv This assert also hit in test vvvv
  assert((PossibleSumZero & Known) == (PossibleSumOne & Known) &&
         "known bits of sum differ");
  // ^^^^

  // Compute known bits of the result.
  KnownBits KnownOut;
  KnownOut.Zero = ~std::move(PossibleSumZero) & Known;
  KnownOut.One = std::move(PossibleSumOne) & Known;
  return KnownOut;
}

nikic · 2024-06-06T11:41:53Z

@c8ef Yeah, you should drop that assert.

c8ef · 2024-06-06T13:05:28Z

Though one testing-related change we should do is drop this check:

llvm-project/llvm/unittests/Support/KnownBitsTest.h

Line 29 in 574ab7e

if (Known.hasConflict())

This means the KnownBits implementation will actually get tested with conflict values, so we can be sure this doesn't produce unexpected crashes somewhere.

If we remove this check, it appears that many tests are failing. Currently, I am skipping values that have conflicts, but I am unsure if this is the correct approach. Please inform me if there are any issues.

RKSimon · 2024-06-06T13:43:02Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

@@ -4212,7 +4212,6 @@ KnownBits SelectionDAG::computeKnownBits(SDValue Op, const APInt &DemandedElts,
    break;
  }

-  assert(!Known.hasConflict() && "Bits known to be one AND zero?");


Maybe just focus on the middle-end initially and then look at removing the DAG/GlobalISel assertions later?

I feel like it's safer to get rid of it everyone at once.

nikic · 2024-06-06T13:56:54Z

llvm/unittests/Support/KnownBitsTest.cpp

@@ -726,6 +741,9 @@ TEST(KnownBitsTest, SExtInReg) {
  unsigned Bits = 4;
  for (unsigned FromBits = 1; FromBits <= Bits; ++FromBits) {
    ForeachKnownBits(Bits, [&](const KnownBits &Known) {
+      if (Known.hasConflict())
+        return;


I think it would be slightly better to move this check until after the Known.sextInReg() call (and also something similar for the other tests). Basically still call the KnownBits helper to make sure it doesn't assert, but don't check what the result will be, as it could be anything.

nikic · 2024-06-06T14:00:23Z

There are some test failures in pre-commit CI:

  LLVM :: Analysis/ScalarEvolution/ashr.ll
  LLVM :: Analysis/ScalarEvolution/pr76234.ll
  LLVM :: CodeGen/AArch64/GlobalISel/form-bitfield-extract-from-sextinreg.mir
  LLVM :: CodeGen/AMDGPU/ds-sub-offset.ll
  LLVM :: Transforms/InstCombine/select-obo-peo-ops.ll
  LLVM :: Transforms/LoopStrengthReduce/lsr-term-fold-negative-testcase.ll
  LLVM-Unit :: Analysis/./AnalysisTests.exe/ValueTrackingTest/ComputeNumSignBits_PR32045
  LLVM-Unit :: IR/./IRTests.exe/DemandedBitsTest/Add
  LLVM-Unit :: IR/./IRTests.exe/DemandedBitsTest/Sub

I think most of these are due to the various if (!Known.hasConflict()) Known.setAllZero(); check that you dropped. You could either adjust the test expectations, or leave dropping these checks for a later patch, so that this one can be NFC (no functional change).

github-actions · 2024-06-06T16:08:31Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jayfoad · 2024-06-06T16:16:31Z

or leave dropping these checks for a later patch, so that this one can be NFC

I would prefer making this an NFC patch that just drops assertions.

nikic

This looks mostly good to me, but I think some of the setAllZeros() removals should be undone.

nikic · 2024-06-07T09:31:29Z

llvm/lib/Support/KnownBits.cpp

-    // Result is either known Zero or UB. Return Zero either way.
-    // Checking this earlier saves us a lot of special cases later on.
-    Known.setAllZero();
+    // Result is either known Zero or UB.


This setAllZero() call should be retained. Otherwise we would now provide a worse result if the LHS is zero. (It would go from result zero to result unknown).

nikic · 2024-06-07T09:35:20Z

llvm/lib/Support/KnownBits.cpp

@@ -403,8 +391,7 @@ KnownBits KnownBits::lshr(const KnownBits &LHS, const KnownBits &RHS,
  if (Exact) {
    unsigned FirstOne = LHS.countMaxTrailingZeros();
    if (FirstOne < MinShiftAmount) {
-      // Always poison. Return zero because we don't like returning conflict.
-      Known.setAllZero();
+      // Always poison.


Similar here and in the ashr implementation. We go from returning zero to returning unknown, which is responsible for the regressions in SCEV tests where we go from range [0, 1) to full-set.

The correct replacement would be something like setConflict(), but given that we don't have that yet and wouldn't be able to make use of it for folding I think it's best to leave these cases alone.

It appears that in the current version(
cfe2878), the testcase llvm/test/Analysis/ScalarEvolution/pr76234.ll is still experiencing a regression from [0, 1) to full-set. Is this the intended behavior?

�_bk;t=1717760175285�# | <stdin>:4:2: note: possible intended match here �_bk;t=1717760175285�# | --> %B9 U: full-set S: full-set �_bk;t=1717760175285�# | ^ �_bk;t=1717760175285�# | �_bk;t=1717760175285�# | Input file: <stdin> �_bk;t=1717760175285�# | Check file: C:\ws\src\llvm\test\Analysis\ScalarEvolution\pr76234.ll �_bk;t=1717760175285�# | �_bk;t=1717760175285�# | -dump-input=help explains the following input dump. �_bk;t=1717760175285�# | �_bk;t=1717760175285�# | Input was: �_bk;t=1717760175285�# | <<<<<< �_bk;t=1717760175285�# | 1: Printing analysis 'Scalar Evolution Analysis' for function 'PR76234': �_bk;t=1717760175285�# | 2: Classifying expressions for: @PR76234 �_bk;t=1717760175285�# | 3: %B9 = shl i896 0, -264147265567832623176169892458258303259423663018060761063980354513336951278362429737208627943828593947337197496628564339441173779751342768625269489231469788454193341999502542084365758838213220526512116454105594202074014146375780869419198449383518238244769290448868999168 �_bk;t=1717760175285�# | next:9'0 X error: no match found �_bk;t=1717760175285�# | 4: --> %B9 U: full-set S: full-set �_bk;t=1717760175285�# | next:9'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ �_bk;t=1717760175285�# | next:9'1 ? possible intended match �_bk;t=1717760175285�# | 5: %B39 = ashr i896 %B9, 1 �_bk;t=1717760175285�# | next:9'0 ~~~~~~~~~~~~~~~~~~~~~~~~~

Hm, I see. In that case I think it would be best to just revert all the setAllZero() related changes. We will have to update consuming code first so it can do something useful with conflict values before we start generating them.

nikic · 2024-06-07T10:26:31Z

llvm/lib/Support/KnownBits.cpp

@@ -448,8 +433,7 @@ KnownBits KnownBits::ashr(const KnownBits &LHS, const KnownBits &RHS,
    MinShiftAmount = 1;
  if (LHS.isUnknown()) {
    if (MinShiftAmount == BitWidth) {
-      // Always poison. Return zero because we don't like returning conflict.
-      Known.setAllZero();
+      // Always poison.


This one, and the next one as well.

nikic

LGTM

jayfoad · 2024-06-07T14:58:39Z

llvm/unittests/IR/DemandedBitsTest.cpp

@@ -38,8 +38,10 @@ static void TestBinOpExhaustive(Fn1 PropagateFn, Fn2 EvalFn) {

          APInt AB1R = PropagateFn(0, AOut, Known1Redacted, Known2Redacted);
          APInt AB2R = PropagateFn(1, AOut, Known1Redacted, Known2Redacted);
-          EXPECT_EQ(AB1, AB1R);
-          EXPECT_EQ(AB2, AB2R);
+          if (!Known1Redacted.hasConflict() && !Known2Redacted.hasConflict()) {


Is this change still needed in this version of the patch?

Confirm it is still needed.

jayfoad · 2024-06-07T15:00:31Z

llvm/unittests/Support/KnownBitsTest.cpp

    });
  }
 }

 static void testBinaryOpExhaustive(StringRef Name, BinaryBitsFn BitsFn,
                                   BinaryIntFn IntFn,
-                                   bool CheckOptimality = true,
-                                   bool RefinePoisonToZero = false) {


Should restore this RefinePoisonToZero stuff, since you have restored all the "if conflict return zero" cases.

I will handle it in another pr.

jayfoad · 2024-06-07T15:03:17Z

llvm/unittests/Support/KnownBitsTest.cpp

@@ -653,8 +656,10 @@ TEST(KnownBitsTest, GetMinMaxVal) {
      Min = APIntOps::umin(Min, N);
      Max = APIntOps::umax(Max, N);
    });
-    EXPECT_EQ(Min, Known.getMinValue());
-    EXPECT_EQ(Max, Known.getMaxValue());
+    if (!Known.hasConflict()) {


If there's a conflict you could immediately "continue" at the top of the loop to save some time. Same in the next three functions below.

#94568 (comment)

But in this case you are not calling Known.anything() before checking for conflict and bailing out!

Hm, yes, I guess we should store Known.getMinValue() etc in a variable first, before the check. Or do people think there is not much value in checking all the functions with conflict inputs?

Or do people think there is not much value in checking all the functions with conflict inputs?

I'm not sure how much value there is right now.

At some point we should check that KnownBits functions return conflict when they ought to, and part of that would definitely involve testing them on conflict inputs.

These particular APIs don't return KnownBits, so they don't really have any specific behavior they should exhibit for conflict inputs beyond "don't assert".

jayfoad · 2024-06-07T15:04:53Z

llvm/unittests/Support/KnownBitsTest.cpp

@@ -752,8 +765,10 @@ TEST(KnownBitsTest, CommonBitsSet) {
          HasCommonBitsSet |= N1.intersects(N2);
        });
      });
-      EXPECT_EQ(!HasCommonBitsSet,
-                KnownBits::haveNoCommonBitsSet(Known1, Known2));
+      if (!Known1.hasConflict() && !Known2.hasConflict()) {


I believe this is necessary because we permit conflicts in test generation. Therefore, even if we revert the set to zero, these tests will only pass when there are no conflicts.

I meant that you could save some time here by testing for conflict as the first thing in the loop body, before the nested ForeachNumInKnownBits loops.

remove hasConflict() assertions

87a6753

llvmbot added the llvm:support label Jun 6, 2024

remove more assert

98b3368

c8ef requested a review from nikic as a code owner June 6, 2024 08:06

llvmbot added backend:X86 llvm:globalisel llvm:SelectionDAG SelectionDAGISel as well llvm:ir llvm:analysis llvm:transforms labels Jun 6, 2024

clang-format

5878f24

c8ef commented Jun 6, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp Show resolved Hide resolved

CedricSwa mentioned this pull request Jun 6, 2024

[KnownBits] Remove hasConflict() assertions #94436

Closed

nikic reviewed Jun 6, 2024

View reviewed changes

llvm/lib/Analysis/ValueTracking.cpp Show resolved Hide resolved

llvm/lib/IR/ConstantRange.cpp Show resolved Hide resolved

address CR comment

3726812

c8ef added 2 commits June 6, 2024 20:53

pass test

10b0837

remove a test arg

7cfe894

c8ef requested a review from nikic June 6, 2024 13:05

RKSimon reviewed Jun 6, 2024

View reviewed changes

nikic reviewed Jun 6, 2024

View reviewed changes

c8ef added 2 commits June 6, 2024 23:21

fix test in DemandBitsTest

4e550dc

adjust ValueTrackingTest(revert D150648)

5896305

clang-format

afd2123

c8ef added 2 commits June 7, 2024 12:48

regen test

3faba75

refactor test

1f7034c

c8ef requested a review from nikic June 7, 2024 06:36

nikic reviewed Jun 7, 2024

View reviewed changes

c8ef added 2 commits June 7, 2024 18:21

revert some setZero

4c9696a

format

7f965e2

nikic reviewed Jun 7, 2024

View reviewed changes

c8ef added 3 commits June 7, 2024 18:30

revert some setZero

6f48b46

revert testcase

cfe2878

revert zero

f44acee

nikic approved these changes Jun 7, 2024

View reviewed changes

jayfoad reviewed Jun 7, 2024

View reviewed changes

nikic merged commit b25b1db into llvm:main Jun 7, 2024
5 of 6 checks passed

jayfoad reviewed Jun 7, 2024

View reviewed changes

c8ef deleted the KnownBits branch June 7, 2024 15:07

HerrCai0907 mentioned this pull request Jun 13, 2024

tidy #95384

Closed

[KnownBits] Remove hasConflict() assertions #94568

[KnownBits] Remove hasConflict() assertions #94568

Conversation

c8ef commented Jun 6, 2024

llvmbot commented Jun 6, 2024 • edited Loading

nikic commented Jun 6, 2024

c8ef commented Jun 6, 2024

nikic commented Jun 6, 2024

nikic commented Jun 6, 2024

c8ef commented Jun 6, 2024 • edited Loading

jayfoad commented Jun 6, 2024

c8ef commented Jun 6, 2024

nikic commented Jun 6, 2024

c8ef commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Jun 6, 2024

github-actions bot commented Jun 6, 2024 • edited Loading

jayfoad commented Jun 6, 2024

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[KnownBits] Remove `hasConflict()` assertions #94568

[KnownBits] Remove `hasConflict()` assertions #94568

llvmbot commented Jun 6, 2024 •

edited

Loading

c8ef commented Jun 6, 2024 •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading