[PATCH] Clarify that the bypassSlowDivision optimization operates on a single BB.

[Hal Finkel via llvm-commits]

----- Original Message -----
> From: "Justin Lebar" <jlebar at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>, "llvm-commits" <llvm-commits at lists.llvm.org>
> Cc: "Tyler Nowicki" <tyler.nowicki at gmail.com>
> Sent: Thursday, December 10, 2015 4:00:47 PM
> Subject: Re: [PATCH] Clarify that the bypassSlowDivision optimization operates on a single BB.
> 
> Hi, thank you for the feedback, Hal.
> 
> I tried making this change, which I agree would be a big readability
> improvement.  But looking more closely at the code, I see that
> bypassSlowDivision is modifying the Function::iterator it's passed.
>  I
> think it's doing this to avoid running the bypassSlowDivision
> optimization on the BBs added by bypassSlowDivision itself.

We can, in a sense, emulate std::vector::erase, and have it return an Instruction *, which is the next one after any new instructions added by bypassSlowDivision itself. The caller can then use this pointer to reset any iterator it might be using.

> 
> It's not clear to me what's the right way to fix this.  We could make
> bypassSlowDivision operate over a whole Function, but that feels kind
> of wrong, since bypassSlowDivision can sanely be applied to a single
> BB.
> 
> Definitely if we leave things as-is, the comment on
> bypassSlowDivision
> should be updated to indicate how it modifies the Function::iterator
> it's passed.

Agreed.

 -Hal

> 
> Thank you again for your help and patience.
> 
> -Justin
> 
> 
> On Wed, Dec 9, 2015 at 8:55 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > Hi Justin,
> >
> > This is better. Our general convention is to use BasicBlock * and
> > Instruction * on interface boundaries. I think using those instead
> > of the iterator types would make things even clearer (and more
> > consistent with our general conventions).
> >
> >  -Hal
> >
> > ----- Original Message -----
> >> From: "Justin Lebar" <jlebar at google.com>
> >> To: "Hal Finkel" <hfinkel at anl.gov>, "Tyler Nowicki"
> >> <tyler.nowicki at gmail.com>
> >> Sent: Wednesday, December 9, 2015 4:36:25 PM
> >> Subject: [PATCH] Clarify that the bypassSlowDivision optimization
> >> operates on a single BB.
> >>
> >> Remove the Function& arg to bypassSlowDivision -- it isn't
> >> necessary,
> >> and makes it look like this function operates on a whole function
> >> rather
> >> than on one basic block.
> >>
> >> Also update some comments to be more explicit.
> >> ---
> >>  include/llvm/Transforms/Utils/BypassSlowDivision.h |  8 ++--
> >>  lib/CodeGen/CodeGenPrepare.cpp                     |  2 +-
> >>  lib/Transforms/Utils/BypassSlowDivision.cpp        | 53
> >>  ++++++++++------------
> >>  3 files changed, 29 insertions(+), 34 deletions(-)
> >>
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> 

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