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

[Justin Lebar via llvm-commits]

Now that I have phabricator/arc set up, moving this review over there
in the hopes that it will be a bit easier for everyone.

http://reviews.llvm.org/D15633

-Justin

On Mon, Dec 14, 2015 at 9:59 AM, Justin Lebar <jlebar at google.com> wrote:
> 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
>>
>>
>>
>>