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

[Nikolai Bozhenov via Phabricator via llvm-commits]

n.bozhenov added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:346
+  FastDivInserter FDI(BypassWidths);
+  FastDivInsertionTask Task;
 
----------------
jlebar wrote:
> n.bozhenov wrote:
> > jlebar wrote:
> > > I'm totally behind the notion of creating classes that encapsulate our temporary state.  But I'm not wild about the way we use FastDivInsertionTask as basically a bucket of *mutable* state.  We have had really nasty bugs in LLVM with similar designs where we forget to reset one piece of the mutable state.  (I think Chandler was saying that some pass effectively had a global cost cap instead of a per-function cost cap, because they just forgot to set the cost to 0 at the end of the function.)
> > > 
> > > I went through a few iterations, and the one that seems best to me right now is to get rid of FastDivInserter, which is already very simple, and make FastDivInsertionTask a one-shot class.
> > > 
> > >   DivCacheTy Cache;
> > >   while (Next != nullptr) {
> > >     Instruction *I = Next;
> > >     Next = Next->getNextNode();
> > > 
> > >     if (Value *Replacement = FastDivInsertionTask(I, Cache, BypassWidths).getReplacement()) {
> > >       I->RAUW(Replacement);
> > >       MadeChange |= true;
> > >     }
> > >   }
> > > 
> > > WDYT?
> > Hi Justin,
> > 
> > Thanks for reviewing the code so quickly. And I really like the code sample you suggested. If I understand correctly, your main concern is that in my code the Task object gets reused for different instructions during the pass, isn't it? If this is the case, then moving the code from FastDivInserter into FastDivInsertionTask constructor is indeed one of the possible solutions.
> > 
> > Another approach is to create a new Task object for each instruction inside an FastDivInserter object and never expose it outside. Something like this:
> > 
> > ```
> > // In bypassSlowDivision:
> > FastDivInserter FDI;
> > while (Next != nullptr) {
> >   Instruction *I = Next;
> >   Next = Next->getNextNode();
> >   MadeChange |= FDI.tryReplaceSlowDiv(I);
> > }
> > ```
> > 
> > ```
> > // In FastDivInserter::tryReplaceSlowDiv:
> > FastDivInsertionTask Task(I, BypassWidths);
> > if (!Task.isSlowDivision())
> >   return false;
> > 
> > Value *R = Task.getReplacement(Cache);
> > if (!R)
> >   return false;
> > 
> > I->RAUW(R);
> > return true;
> > ```
> > 
> > Currently I'm playing with these two approaches and will come with an updated patch by tomorrow. Please, comment if you're strongly in favour of one of these approaches.
> > 
> > If I understand correctly, your main concern is that in my code the Task object gets reused for different instructions during the pass, isn't it?
> 
> Yes.
> 
> I'm also a little concerned about the mutable state inside of the Task object used for passing between functions, but I wanted to get this part figured out first.
> 
> > Another approach is to create a new Task object for each instruction inside an FastDivInserter object and never expose it outside
> 
> Right.  If FastDivInserter has nontrivial complexity that it doesn't make sense to push into FastDivInsertionTask or the outer pass function body, this might make sense.  When I looked at it, I didn't think it did.
I have updated the patch. I have got rid of FastDivInserter class. And the Task is not reused any more, but a new object is constructed for each instruction.

As for mutable fields used for passing data between functions, it's possible to get rid of them as well. We could define

```
struct IncomingDivRemPair {
  BasicBlock *BB; //< PHINode predecessor for the following values.
  Value *Quotient;
  Value *Remainder;
};
```

and return such a structure by value from createSlowBB and createFastBB methods. Later, we could pass a pair of such structures into createDivRemPhiNodes to make the latter both more flexible and easier to understand. The only reason I haven't done this yet is that I don't like very much returning structures by value. Do you think it is worth doing?



https://reviews.llvm.org/D29896