[PATCH] Rename PerBBDivCache to DivCache in BypassSlowDivision.

[Justin Lebar via llvm-commits]

Thank you, Tyler and Hal.  Sent out a new patch.

-Justin

On Wed, Dec 9, 2015 at 10:07 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- 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