[PATCH] D28199: [BypassSlowDivision] Use ValueTracking to simplify run-time checks

Justin Lebar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Dec 31 10:08:11 PST 2016


jlebar added a comment.

I'm excited to see what this does for our GPU kernels.



================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:48
+
+  enum ValRng { VALRNG_SHORT, VALRNG_UNKNOWN, VALRNG_LONG };
 }
----------------
Please define what these mean.  (The notion of "long" and "short" are important to this patch, but we don't define them anywhere.)


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:48
+
+  enum ValRng { VALRNG_SHORT, VALRNG_UNKNOWN, VALRNG_LONG };
 }
----------------
jlebar wrote:
> Please define what these mean.  (The notion of "long" and "short" are important to this patch, but we don't define them anywhere.)
Suggest renaming the enum to ValueRange -- you can still abbreviate it "VALRNG" in the enumerators.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:78
 
+/// \brief Check if the value may fit the shorter type.
+///
----------------
> Check if an integer value fits into a smaller type.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:82
+/// is known to be too wide. In the former case the runtime check can be
+/// simplified, in the latter case bypassing should be avoided.
+static ValRng getValueRange(Value *Op, IntegerType *BypassType,
----------------
This paragraph doesn't add any additional information IMO -- maybe we should just get rid of it.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:87
+  unsigned longLen = cast<IntegerType>(Op->getType())->getBitWidth();
+  unsigned nHiBits = longLen - shortLen;
+  APInt Zeros(longLen, 0), Ones(longLen, 0);
----------------
LLVM convention is to start all variable names with upper-case letters -- please fix here and elsewhere.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:98
+    return VALRNG_SHORT;
+  }
+
----------------
LLVM convention is not to use braces on single-line if statements.  Please fix here and elsewhere.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:121
 
-  // If the numerator is a constant, bail if it doesn't fit into BypassType.
-  if (ConstantInt *ConstDividend = dyn_cast<ConstantInt>(Dividend))
-    if (ConstDividend->getValue().getActiveBits() > BypassType->getBitWidth())
-      return false;
+  ValRng DividendRng = getValueRange(Dividend, BypassType, DL);
+  if (DividendRng == VALRNG_LONG)
----------------
Nit, this looks like "dividend random number generator" to me.  Spelling out "Range" would only be an extra two chars -- maybe we should do that for both ValRng and Dividend/DivisorRange.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:139
+    Value *trResult = UseDivOp ? Builder.CreateUDiv(trDividend, trDivisor)
+                               : Builder.CreateURem(trDividend, trDivisor);
+    Value *Result = Builder.CreateZExt(trResult, I->getType());
----------------
Suggest s/tr/Trunc/.  In any case please use uppercase.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:149
   BasicBlock *SuccessorBB = MainBB->splitBasicBlock(I);
-
-  // Add new basic block for slow divide operation
-  BasicBlock *SlowBB =
-      BasicBlock::Create(F->getContext(), "", MainBB->getParent(), SuccessorBB);
-  SlowBB->moveBefore(SuccessorBB);
-  IRBuilder<> SlowBuilder(SlowBB, SlowBB->begin());
-  Value *SlowQuotientV;
-  Value *SlowRemainderV;
-  if (UseSignedOp) {
-    SlowQuotientV = SlowBuilder.CreateSDiv(Dividend, Divisor);
-    SlowRemainderV = SlowBuilder.CreateSRem(Dividend, Divisor);
+  Value *ZeroV = ConstantInt::get(I->getType(), 0);
+
----------------
I think it would be clearer if we didn't have this variable and just wrote out `ConstantInt::get(I->getType(), 0)` in the two places where we need it.  As-is, a reader is going to have to use Vim's `#` to figure out what this variable is, since the function is so long and the uses are so far from the declaration.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:151
+
+  // If the division is unsigned and Dividend is known to be short then either
+  // 1) Divisor is less or equal to Dividend and the result can be computed
----------------
Long prepositional phrases at the beginning of a sentence should be followed by a comma.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:152
+  // If the division is unsigned and Dividend is known to be short then either
+  // 1) Divisor is less or equal to Dividend and the result can be computed
+  //    with a short division.
----------------
comma before "and" (separating independent clauses)


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:154
+  //    with a short division.
+  // 2) Divisor is greater than Dividend. Then the (long) division is not
+  //    needed at all. In this case the quotient is 0 and the remainder is
----------------
> In this case, no division is needed at all: The quotient is 0 and the remainder is equal to Dividend.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:156
+  //    needed at all. In this case the quotient is 0 and the remainder is
+  //    equal to Dividend.
+  bool ShortCut = DividendShort && !UseSignedOp;
----------------
This comment is missing the (really cool) punch line:

> So instead of checking at runtime whether Divisor fits into BypassType, we emit a runtime check to differentiate between these two cases.  This lets us entirely avoid a long div.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:157
+  //    equal to Dividend.
+  bool ShortCut = DividendShort && !UseSignedOp;
+
----------------
"Shortcut" is one word, but also, this control flow is very complicated.

For example, it's confusing that the first `if (ShortCut)` branch, which is in the "basic block for slow divide operation", actually does something very fast.  One could add comments, but they're going to be pretty complicated.  ("New BB for long divide, which we bypass if possible, see definition of ShortCut...")

It seems to me we really want to say

  if (DividendShort && !UseSignedOp) {
    // Do one thing.
  } else {
    // Do something different.
  }

Could we refactor the code so we can write it this way?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:232
+  } else {
+    // Combine operands into a single value with OR for value testing below
+    Value *OrV = nullptr;
----------------
Suggest commenting on what the whole "else" branch is doing:

> Check at runtime whether Dividend and Divisor fit in BypassType.

Mentioning the "OR" is more confusing than helpful, I think, because we don't always emit the OR.


================
Comment at: test/CodeGen/X86/bypass-slow-division-64.ll:67
+
+; A simplified run-time check when divisor is known to be short.
+define i64 @Test_check_one_operand(i64 %a, i32 %b) nounwind {
----------------
The first comment added here is good, but then the ones below are all fragments, which is confusing.  Can we make them complete sentences?


https://reviews.llvm.org/D28199





More information about the llvm-commits mailing list