[PATCH] Rename PerBBDivCache to DivCache in BypassSlowDivision.

[Hal Finkel via llvm-commits]

----- Original Message -----
> From: "Justin Lebar" <jlebar at google.com>
> To: "Tyler Nowicki" <tyler.nowicki at gmail.com>
> Cc: "Hal Finkel" <hfinkel at anl.gov>, llvm-commits at lists.llvm.org, "preston gurd" <preston.gurd at intel.com>
> Sent: Wednesday, December 9, 2015 11:47:43 AM
> Subject: Re: [PATCH] Rename PerBBDivCache to DivCache in BypassSlowDivision.
> 
> Oh, I see.  We even iterate over the BBs in
> CodeGenPrepare::runOnFunction, it's quite clear.
> 
> Since I managed to convince one other person that there was something
> up here, I may send a patch to change the comments slightly, if
> that's
> OK.
> 
> Sorry for the noise.

Part of what is confusing here is that:

// bypassSlowDivision - This optimization identifies DIV instructions that can
// be profitably bypassed and carried out with a shorter, faster divide.
bool llvm::bypassSlowDivision(Function &F,
                              Function::iterator &I,
                              const DenseMap<unsigned int, unsigned int> &BypassWidths) {

kind of looks like it operates over a function, not a single basic block. bypassSlowDivision's F parameter will always be *I->getParent(), and there is no reason it should be a separate parameter. Can you just remove the F parameter? I think that will make things clearer?

Thanks again,
Hal

> 
> -Justin
> 
> On Wed, Dec 9, 2015 at 8:46 AM, Tyler Nowicki
> <tyler.nowicki at gmail.com> wrote:
> > Hi Justin, Hal,
> >
> >
> >
> > It is a BB level cache. bypassSlowDivision() is executed once per
> > BasicBlock. Notice that the DivCache is locally defined in
> > bypassSlowDivision(). So the cache would be empty when %Ret2 is
> > encountered.
> >
> >
> >
> > Tyler
> >
> >
> > From: Hal Finkel
> > Sent: December 9, 2015 7:16 AM
> > To: Justin Lebar
> > Cc: preston gurd;llvm-commits;Tyler Nowicki
> > Subject: Re: [PATCH] Rename PerBBDivCache to DivCache in
> > BypassSlowDivision.
> >
> >
> >
> >
> >
> > Hi Justin,
> >
> >
> >
> > You seem to be right, but the code does not seem to be setup
> > correctly for a
> > function-level cache. Specifically, it seems to be missing a
> > dominance check
> > before reusing the result of the previous division expansion. Thus,
> > if we
> > have:
> >
> >
> >
> > %Dividend = ...
> >
> > %Divisor = ...
> >
> >
> >
> > if (%whatever) {
> >
> >   %Ret = %Dividend / %Divisor;
> >
> > } else {
> >
> >   %Ret2 = %Dividend / %Divisor;
> >
> > }
> >
> >
> >
> > what prevents this code from "reusing" the expansion for %Ret upon
> > encountering %Ret2?
> >
> >
> >
> > -Hal
> >
> >
> >
> > ----- Original Message -----
> >
> >> From: "Justin Lebar via llvm-commits"
> >> <llvm-commits at lists.llvm.org>
> >
> >> To: "llvm-commits" <llvm-commits at lists.llvm.org>
> >
> >> Cc: "preston gurd" <preston.gurd at intel.com>
> >
> >> Sent: Tuesday, December 8, 2015 12:12:22 PM
> >
> >> Subject: [PATCH] Rename PerBBDivCache to DivCache in
> >> BypassSlowDivision.
> >
> >>
> >
> >> This hashtable is used for the whole function, not just the BB.
> >
> >>
> >
> >> No functional changes.
> >
> >> ---
> >
> >>  lib/Transforms/Utils/BypassSlowDivision.cpp | 13 ++++++-------
> >
> >>  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> >>
> >
> >> _______________________________________________
> >
> >> llvm-commits mailing list
> >
> >> llvm-commits at lists.llvm.org
> >
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> >>
> >
> >
> >
> > --
> >
> > Hal Finkel
> >
> > Assistant Computational Scientist
> >
> > Leadership Computing Facility
> >
> > Argonne National Laboratory
> >
> >
> >
> >
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory