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

[Justin Lebar via llvm-commits]

Hi, Hal, Tyler.  Thank you again for your feedback.

Please have a look at this latest patch.  I've gotten rid of passing
of iterators by reference.

I don't know if this is idiomatic LLVM code, but I find it much easier
to follow.  Before you had to have global knowledge of how callees
changed the iterators in order to read one of these loops -- now the
control flow is all contained in the same function as the loop itself.

I think this is correct, but the old code was somewhat complicated;
please have a careful look for bugs.

-Justin


On Thu, Dec 10, 2015 at 3:37 PM, Tyler Nowicki <tyler.nowicki at gmail.com> wrote:
>
>
> 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 --------------
A non-text attachment was scrubbed...
Name: 0001-Clarify-that-the-bypassSlowDivision-optimization-ope.patch
Type: application/octet-stream
Size: 11488 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151214/19319959/attachment.obj>