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

[Tyler Nowicki via llvm-commits]

Hi Justin, Hal,

The patch LGTM.

When the given BB is split the iterator is set to a new successor BB. This way when control is returned to bypassSlowDivision() it can begin from the top of the successor BB or from some middle point in the given BB if a fast div was reused. And when bypassSlowDivision() returns the iterator is set to the last successor BB. As Justin pointed out this prevents bypassSlowDivision() from being run on the new BBs.

Tyler

From: Hal Finkel
Sent: December 10, 2015 4:10 PM
To: Justin Lebar
Cc: Tyler Nowicki;llvm-commits
Subject: Re: [PATCH] Clarify that the bypassSlowDivision optimization operateson a single BB.


----- 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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151210/0b7c57f9/attachment.html>