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

[Nikolai Bozhenov via Phabricator via llvm-commits]

n.bozhenov marked 24 inline comments as done.
n.bozhenov added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:84
+  IntegerType *BypassType;
+  BasicBlock *MainBB;
+  Value *Dividend;
----------------
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.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:181
+    // If previous instance does not exist, try to insert fast div
+    insertFastDiv();
+    CacheI = Cache.find(Key);
----------------
jlebar wrote:
> It seems like much less of a layering violation to me if insertFastDiv (possibly renamed to `createFastDivOrRem`) would return Optional<std::pair<Value*, Value*>>.  Then we would update the cache here.
Right. I have moved all the caching logic into this routine. As a result, I could get rid of Cache field in the class. Now it's only passed here as a method argument.


================
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.
----------------
jlebar wrote:
> Perhaps we should update this comment.
Not sure what's wrong with the comment.


https://reviews.llvm.org/D29896