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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:346
+  FastDivInserter FDI(BypassWidths);
+  FastDivInsertionTask Task;
 
----------------
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.


https://reviews.llvm.org/D29896