[PATCH] D29896: [BypassSlowDivision] Refactor fast division insertion logic (NFC)

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

Sorry to keep going back and forth on this.  I'm almost happy, but still have some meaty comments.



================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:84
+  IntegerType *BypassType;
+  BasicBlock *MainBB;
+  Value *Dividend;
----------------
n.bozhenov wrote:
> jlebar wrote:
> > Similarly for the MainBB field.
> Things get more difficult with MainBB. After splitting the basic block it is not obvious any more what basic block SlowDivOrRem->getParent() will return. So, I believe it's better to keep MainBB member in the class.
sgtm.  It's a lot more clear now.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:306
 
-// bypassSlowDivision - This optimization identifies DIV instructions in a BB
-// that can be profitably bypassed and carried out with a shorter, faster
-// divide.
-bool llvm::bypassSlowDivision(
-    BasicBlock *BB, const DenseMap<unsigned int, unsigned int> &BypassWidths) {
-  DivCacheTy DivCache;
+/// This optimization identifies DIV instructions in a BB that can be profitably
+/// bypassed and carried out with a shorter, faster divide.
----------------
n.bozhenov wrote:
> jlebar wrote:
> > Perhaps we should update this comment.
> Not sure what's wrong with the comment.
I could have sworn it used to say "identifies division instructions", but now it says "DIV/REM" instructions, which is all I wanted.  We're good here.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:94
+class FastDivInsertionTask {
+  bool IsSlowDivision = false;
+  Instruction *SlowDivOrRem = nullptr;
----------------
I think we could still be more explicit about which variables have to do with division specifically and which have to do with div/rem.  Right now it's confusing that IsSlowDivision actually means "should we try to generate a fast div/rem for SlowDivOrRem?".  Similarly it's confusing that `isSignedDiv` means "is SlowDivOrRem a signed div or rem?".

Maybe rename "IsSlowDivision" to "IsValid" or "IsValidTask"?  And `isSignedDiv` to `isSignedOp` to match `isDivisionOp`?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:106
+
+  bool isSignedDiv() {
+    return SlowDivOrRem->getOpcode() == Instruction::SDiv ||
----------------
isSignedOp()?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:174
+    // If previous instance does not exist, try to insert fast div.
+    DivRemResult DivRem = insertFastDivAndRem();
+    // Bail out if insertFastDivAndRem has failed.
----------------
It's kind of confusing that DivRemResult is "nullable".  While the result of insertFastDivAndRem is nullable, the value inside of the cache must not be null.

Seems like it would be clearer if we made DivRemResult not nullable and then returned an optional<DivRemResult> from insertFastDivAndRem.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:181
+    std::pair<DivOpInfo, DivRemResult> KeyVal(Key, DivRem);
+    CacheI = Cache.insert(KeyVal).first;
   }
----------------
Perhaps just

  CacheI = Cache.Insert({Key, DivRem}).first

?  Not sure we need the comment if you write it like this, either.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:188
+  else
+    return Value.Remainder;
+}
----------------
Nit, perhaps clearer as a ternary:

  return isDivisionOp() ? Value.Quotient : Value.Remainder;


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:193
+/// SuccessorBB.
+IncomingDivRemPair FastDivInsertionTask::createSlowBB(BasicBlock *SuccessorBB) {
+  IncomingDivRemPair DivRemPair;
----------------
"IncomingDivRemPair" is kind a confusing name, because the values are "outgoing" from one block and "incoming" to another.

It's also subtly different from DivRemResult.

It's going to be true that at least one of the Value*s inside our IncomingDivRemPair is an Instruction, right?  If so, maybe we should

  * Rename DivRemResult to DivRemPair
  * Get rid of IncomingDivRemPair
  * Add a DivRemPair::basicBlock() method that gets the BB from its Values (assuming that at least one is an Instruction).


https://reviews.llvm.org/D29896