[CodeGen] Remove checks for vectors in unsigned division prior to computing leading zeros

[AZero13]

It turns out we can safely use DAG.computeKnownBits(N0).countMinLeadingZeros() with constant legal vectors, so remove the check for it.

[llvmbot]

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-llvm-selectiondag

Author: AtariDreams (AtariDreams)

Changes

It turns out that the function already exits early if N1 is not a constant or is a vector, so simplify.

---

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

1 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+10-10)

diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index c3a20b5044c5f..7e38a3607cdd1 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -6483,15 +6483,7 @@ SDValue TargetLowering::BuildUDIV(SDNode *N, SelectionDAG &DAG,
 
   // Try to use leading zeros of the dividend to reduce the multiplier and
   // avoid expensive fixups.
-  // TODO: Support vectors.
-  unsigned LeadingZeros = 0;
-  if (!VT.isVector() && isa<ConstantSDNode>(N1)) {
-    assert(!isOneConstant(N1) && "Unexpected divisor");
-    LeadingZeros = DAG.computeKnownBits(N0).countMinLeadingZeros();
-    // UnsignedDivisionByConstantInfo doesn't work correctly if leading zeros in
-    // the dividend exceeds the leading zeros for the divisor.
-    LeadingZeros = std::min(LeadingZeros, N1->getAsAPIntVal().countl_zero());
-  }
+  unsigned KnownLeadingZeros = DAG.computeKnownBits(N0).countMinLeadingZeros();
 
   bool UseNPQ = false, UsePreShift = false, UsePostShift = false;
   SmallVector<SDValue, 16> PreShifts, PostShifts, MagicFactors, NPQFactors;
@@ -6509,8 +6501,16 @@ SDValue TargetLowering::BuildUDIV(SDNode *N, SelectionDAG &DAG,
       PreShift = PostShift = DAG.getUNDEF(ShSVT);
       MagicFactor = NPQFactor = DAG.getUNDEF(SVT);
     } else {
+
+      // Try to use leading zeros of the dividend to reduce the multiplier and
+      // avoid expensive fixups.
+      unsigned LeadingZeros = DAG.computeKnownBits(N0).countMinLeadingZeros();
+      // UnsignedDivisionByConstantInfo doesn't work correctly if leading zeros
+      // in the dividend exceeds the leading zeros for the divisor.
+      LeadingZeros = std::min(LeadingZeros, N1->getAsAPIntVal().countl_zero());
       UnsignedDivisionByConstantInfo magics =
-          UnsignedDivisionByConstantInfo::get(Divisor, LeadingZeros);
+          UnsignedDivisionByConstantInfo::get(
+              Divisor, std::min(KnownLeadingZeros, Divisor.countl_zero()));
 
       MagicFactor = DAG.getConstant(magics.Magic, dl, SVT);
 

[topperc]

There's only an early exit for vector in the TargetLowering code if the type isn't legal.

[topperc]

I don't think this patch is NFC

[topperc]

This patch changes the generated code for this test for x86-64

define <4 x i32> @foo(<4 x i32> %x) {                                            
  %a = and <4 x i32> %x, <i32 255, i32 255, i32 255, i32 255>                    
  %b = udiv <4 x i32> %a, <i32 7, i32 7, i32 7, i32 7>                           
  ret <4 x i32> %b                                                               
} 

[topperc]

It turns out we can safely use DAG.computeKnownBits(N0).countMinLeadingZeros() with constant legal vectors, so remove the check for it.

It is safe, but you have to write tests for it.

[AZero13]

define <4 x i32> @foo(<4 x i32> %x) {

I actually added this check to avoid unforeseen bugs and turns out that was bad on my part. But yeah I will add the test now.

[topperc]

define <4 x i32> @foo(<4 x i32> %x) {

I actually added this check to avoid unforeseen bugs and turns out that was bad on my part. But yeah I will add the test now.

What check did you add? The original code in TargetLowering was written by me. https://reviews.llvm.org/D140750 I specifically didn't do vectors at the time because I hadn't thought through the math. As I wrote at the time:

"Wasn't sure if we need to apply the clip by divisor leading zeros uniformly across the whole vector when there are different divisors which would require another loop over the divisors. Or if we could treat each element individually and apply a clip inside BuildUDIVPattern."

[topperc]

Add back request changes until tests are added.

[AZero13]

Add back request changes until tests are added.

define <4 x i32> @foo(<4 x i32> %x) {

I actually added this check to avoid unforeseen bugs and turns out that was bad on my part. But yeah I will add the test now.

What check did you add? The original code in TargetLowering was written by me. https://reviews.llvm.org/D140750 I specifically didn't do vectors at the time because I hadn't thought through the math. As I wrote at the time:

"Wasn't sure if we need to apply the clip by divisor leading zeros uniformly across the whole vector when there are different divisors which would require another loop over the divisors. Or if we could treat each element individually and apply a clip inside BuildUDIVPattern."

My bad, I was confusing selectiondag and globalisel

[topperc]

PR title needs to be updated. As we've now shown, they were never redundant.

[AZero13]

PR title needs to be updated. As we've now shown, they were never redundant.

Fixed!

[topperc]

LGTM

[AZero13]

@topperc This should be good to go now! Thank you!

Please note I do not have commit perms...

[topperc]

LGTM