[KnownBits] Return `0` for poison {s,u}div inputs

[goldstein.w.n]

Summary

It seems consistent to always return zero for known poison rather than
varying the value. We do the same elsewhere.

[goldstein.w.n]

llvm/test/CodeGen/WebAssembly/pr59626.ll

18

I think this is a meaningless regression. The IR being tests evaluates to just ret i8 poison @luke is this okay?

[luke]

llvm/test/CodeGen/WebAssembly/pr59626.ll

18

Can you point to the IR test that's evaluating to just poison? As long as this test case isn't crashing then I'm ok, but I'm probably not the right person to ask about KnownBits. I'm surprised this is the only test case that is affected!

[goldstein.w.n]

llvm/test/CodeGen/WebAssembly/pr59626.ll

18

Can you point to the IR test that's evaluating to just poison?

This test @f:
https://alive2.llvm.org/ce/z/VDJkEb

As long as this test case isn't crashing then I'm ok, but I'm probably not the right person to ask about KnownBits. I'm surprised this is the only test case that is affected!

I'm just asking if the affects to the wasm codegen is okay.

[goldstein.w.n]

All do INT_MIN / -1 case

[nikic]

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

[goldstein.w.n]

In D150922#4357344, @nikic wrote:

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

[goldstein.w.n]

In D150922#4357610, @goldstein.w.n wrote:

In D150922#4357344, @nikic wrote:

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

Actually testing the impl without this, the zero, zero check will still be needed (just
later on) so would prefer to keep that regardless.

[nikic]

In D150922#4357610, @goldstein.w.n wrote:

In D150922#4357344, @nikic wrote:

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

Is this referring to the code in D150923? That is we do the early isZero() check here and then divComputeLowBit() from that patch has an implicit precondition that the denominator is not zero? Adding an explicit < BitWidth check there seems more obvious to me, unless it's more than that?

[goldstein.w.n]

In D150922#4357670, @nikic wrote:

In D150922#4357610, @goldstein.w.n wrote:

In D150922#4357344, @nikic wrote:

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

Is this referring to the code in D150923? That is we do the early isZero() check here and then divComputeLowBit() from that patch has an implicit precondition that the denominator is not zero? Adding an explicit < BitWidth check there seems more obvious to me, unless it's more than that?

Need multiple (although I could just move this isZero() checks to divComputeLowBit.

[goldstein.w.n]

Remove cases that don't help cleanup rest of the code

[goldstein.w.n]

Rebase

[nikic]

LGTM

llvm/lib/Support/KnownBits.cpp

762

nit: Poison -> UB

llvm/lib/Support/KnownBits.cpp

KnownBits KnownBits::sdiv(const KnownBits &LHS, const KnownBits &RHS,
   // Equivalent of `udiv`. We must have caught this before it was folded.
   if (LHS.isNonNegative() && RHS.isNonNegative())
     return udiv(LHS, RHS, Exact);
 
   unsigned BitWidth = LHS.getBitWidth();
   assert(!LHS.hasConflict() && !RHS.hasConflict() && "Bad inputs");
   KnownBits Known(BitWidth);
 
+  if (LHS.isZero() || RHS.isZero()) {
+    // 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();
+    return Known;
+  }
+
   std::optional<APInt> Res;
   if (LHS.isNegative() && RHS.isNegative()) {
     // Result non-negative.
     APInt Denom = RHS.getSignedMaxValue();
     APInt Num = LHS.getSignedMinValue();
     // INT_MIN/-1 would be a poison result (impossible). Estimate the division
     // as signed max (we will only set sign bit in the result).
     Res = (Num.isMinSignedValue() && Denom.isAllOnes())

 }
 
 KnownBits KnownBits::udiv(const KnownBits &LHS, const KnownBits &RHS,
                           bool Exact) {
   unsigned BitWidth = LHS.getBitWidth();
   assert(!LHS.hasConflict() && !RHS.hasConflict());
   KnownBits Known(BitWidth);
 
+  if (LHS.isZero() || RHS.isZero()) {
+    // 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();
+    return Known;
+  }
+
   // We can figure out the minimum number of upper zero bits by doing
   // MaxNumerator / MinDenominator. If the Numerator gets smaller or Denominator
   // gets larger, the number of upper zero bits increases.
   APInt MinDenom = RHS.getMinValue();
   APInt MaxNum = LHS.getMaxValue();
   APInt MaxRes = MinDenom.isZero() ? MaxNum : MaxNum.udiv(MinDenom);
 
   unsigned LeadZ = MaxRes.countLeadingZeros();

llvm/test/CodeGen/WebAssembly/pr59626.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=wasm32-- -mattr=+simd128 | FileCheck --check-prefix=CHECK-32 %s
 ; RUN: llc < %s -mtriple=wasm64-- -mattr=+simd128 | FileCheck --check-prefix=CHECK-64 %s
 
 define i8 @f(ptr %0, ptr %1) {
 ; CHECK-32-LABEL: f:
 ; CHECK-32:         .functype f (i32, i32) -> (i32)
 ; CHECK-32-NEXT:  # %bb.0: # %BB
 ; CHECK-32-NEXT:    local.get 0
 ; CHECK-32-NEXT:    i32.const 0
 ; CHECK-32-NEXT:    i32.store8 2
 ; CHECK-32-NEXT:    local.get 0
 ; CHECK-32-NEXT:    i32.const 0
 ; CHECK-32-NEXT:    i32.store16 0
 ; CHECK-32-NEXT:    local.get 1
+; CHECK-32-NEXT:    i32.const 0
+; CHECK-32-NEXT:    i32.store8 2
+; CHECK-32-NEXT:    local.get 1
 ; CHECK-32-NEXT:    local.get 0
 ; CHECK-32-NEXT:    i8x16.splat
 ; CHECK-32-NEXT:    v128.store16_lane 0, 0
 ; CHECK-32-NEXT:    v128.const 0, 0
 ; CHECK-32-NEXT:    i32x4.extract_lane 0
 ; CHECK-32-NEXT:    # fallthrough-return
 ;
 ; CHECK-64-LABEL: f: