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

[Nikolai Bozhenov via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:193
+/// SuccessorBB.
+IncomingDivRemPair FastDivInsertionTask::createSlowBB(BasicBlock *SuccessorBB) {
+  IncomingDivRemPair DivRemPair;
----------------
jlebar wrote:
> n.bozhenov wrote:
> > jlebar wrote:
> > > "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).
> > I do agree that DivRemResult and IncomingDivRemPair have something in common and it's tempting to get rid of one of them. On the other hand, they serve different purposes and have no complex logic to share. As a result, I believe these two structures should be kept separate one from another. Not sure about if their names are perfect, though.
> > 
> > DivRemResult is a type that is used to cache results of bypassing. Usually, it is a pair of PHINodes. However, in D29897 there is a special case when it is a pair of ZExt instructions.
> > 
> > IncomingDivRemPair structures are used to create two PHINodes in createDivRemPhiNodes. Each of the IncomingDivRemPair arguments describes one of the incoming values for the PHINodes. Again, in D29897 there is a special case when Quotient = Zero and Remainder = Dividend. In this case it would be impossible to derive information about PHINode predecessor from DivRemPair. So, basic block information should be explicitely included somehow into createDivRemPhiNodes arguments.
> > 
> > Moreover, DivOpInfo and DivRemResult are auxilliary types for caching only. They contain only information significant for caching and it is extremely unlikely that any of these types will be extended in future. On the other hand, one may want to pass some additional information to createDivRemPhiNodes in future (e.g. some sort of metadata) and add additional fields to IncomingDivRemPair. It is not a problem only if DivRemResult and IncomingDivRemPair are two different types.
> > 
> Okay, how about we just name them based on what they contain, `QuotRemPair` and `QuotRemWithBB`?
Definitely, such names have the advantage of being absolutely unambiguous.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:45
+
+    DivRemResult() : Quotient(nullptr), Remainder(nullptr) {}
+    DivRemResult(Value *InQuotient, Value *InRemainder)
----------------
jlebar wrote:
> Now we can remove this constructor, right?
Yep.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:172
+    // If previous instance does not exist, try to insert fast div.
+    Optional<DivRemResult> OptDivRem = insertFastDivAndRem();
+    // Bail out if insertFastDivAndRem has failed.
----------------
jlebar wrote:
> It's not really a "div rem" -- it's the *result* of the div and rem, which is a quotient plus a remainder.  Maybe just QuotRem or Result?
Makes sense.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:284
+    // Keep division by a constant for DAGCombiner.
+    return Optional<DivRemResult>();
   }
----------------
jlebar wrote:
> `return None` here and elsewhere.
Right. Thanks.


https://reviews.llvm.org/D29896