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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:194
 
+/// Check if an integer value fits into a smaller type.
+ValueRange FastDivInsertionTask::getValueRange(Value *Op,
----------------
Maybe, "check if an integer type fits into our bypass type"?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:195
+/// Check if an integer value fits into a smaller type.
+ValueRange FastDivInsertionTask::getValueRange(Value *Op,
+                                               const DataLayout &DL) {
----------------
Value *V ?  It's not necessarily an operator -- could be a constant or something.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:201
+
+  APInt Zeros(LongLen, 0), Ones(LongLen, 0);
+
----------------
Should we assert that V has the same width as getSlowType()?  If its type is shorter I think it works out, but if it's longer, does it still work?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:285
 
-  // We should have bailed out above if the divisor is a constant, but the
-  // dividend may still be a constant.  Set OrV to our non-constant operands
-  // OR'ed together.
-  assert(!isa<ConstantInt>(Divisor));
-
-  Value *OrV;
-  if (!isa<ConstantInt>(Dividend))
-    OrV = Builder.CreateOr(Dividend, Divisor);
+  Value *OrV = nullptr;
+  if (Op1 && Op2)
----------------
Would actually prefer not to initialize this variable to null -- this way we'll get a compile warning if it's not initialized in both branches of the if below.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:291
+
+  assert(OrV && "Nothing to check");
 
----------------
Maybe change this to assert((Op1 || Op2) && "Nothing to check") and move up to the top of the function?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:314
 
-  // 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 None;
-
-  // Split the basic block before the div/rem.
-  BasicBlock *SuccessorBB = MainBB->splitBasicBlock(SlowDivOrRem);
-  // Remove the unconditional branch from MainBB to SuccessorBB.
-  MainBB->getInstList().back().eraseFromParent();
-  QuotRemWithBB Fast = createFastBB(SuccessorBB);
-  QuotRemWithBB Slow = createSlowBB(SuccessorBB);
-  QuotRemPair Result = createDivRemPhiNodes(Fast, Slow, SuccessorBB);
-  Value *CmpV = insertOperandRuntimeCheck();
-  IRBuilder<> Builder(MainBB, MainBB->end());
-  Builder.CreateCondBr(CmpV, Fast.BB, Slow.BB);
-  return Result;
+  const DataLayout &DL = SlowDivOrRem->getModule()->getDataLayout();
+
----------------
I wonder if getValueRange should get the DL itself -- one less step for us here.


================
Comment at: test/CodeGen/X86/bypass-slow-division-64.ll:95
+  ret i64 %res
+}
+
----------------
I should have been looking more closely at these testcases -- this seems really fragile because it depends on our exact register allocation?  I know that to some degree the registers are fixed because of the calling convention plus the fixed in/out regs of e.g. idiv.  But to the extent that the registers are *not* fixed, I don't think we should be relying on a particular register allocation.  That will just make unnecessary pain for the next person to come along and change the register allocator.


================
Comment at: test/CodeGen/X86/bypass-slow-division-64.ll:105
+; CHECK-NEXT:    shrq $32, %rax
+; CHECK-NEXT:    je .LBB4_1
+; CHECK-NEXT:  # BB#2:
----------------
Similarly here we're relying on the particular label name LBB4_1.  This seems extremely fragile -- the test would break if we added a function above this one?

I think we should either do this right, with FileCheck variable matching (i.e. [[XYZ:some_regex]]), or just CHECK for "je" and ignore the label.


https://reviews.llvm.org/D29897