[PATCH] [InstCombine] Fold endian-independent load sequence into a single load.

[Michael Spencer]

On Thu, Oct 23, 2014 at 2:17 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Ahmed Bougacha" <ahmed.bougacha at gmail.com>
>> To: "Michael Spencer" <bigcheesegs at gmail.com>
>> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Chandler Carruth" <chandlerc at google.com>, "amara emerson"
>> <amara.emerson at arm.com>, "llvm-commits" <llvm-commits at cs.uiuc.edu>, "david majnemer" <david.majnemer at gmail.com>,
>> reviews+D5898+public+a9255b02ad589d6d at reviews.llvm.org
>> Sent: Thursday, October 23, 2014 4:01:43 PM
>> Subject: Re: [PATCH] [InstCombine] Fold endian-independent load sequence into a single load.
>>
>> On Thu, Oct 23, 2014 at 1:00 PM, Michael Spencer
>> <bigcheesegs at gmail.com> wrote:
>> > On Thu, Oct 23, 2014 at 11:48 AM, Hal Finkel <hfinkel at anl.gov>
>> > wrote:
>> >> Chandler, Michael, before we go farther, could you please comment
>> >> on whether this is an optimal place for this optimization? We do
>> >> have a LoadCombine pass, maybe it should really go there?
>> >>
>> >> Ahmed, I think we also need to check for other uses of the
>> >> loads... if they also have other uses, then we likely don't want
>> >> to double-load the same data.
>> >>
>> >>  -Hal
>> >
>> > The load combiner was made to deal with this case, however it's not
>> > enabled right now because load slicing still needs to be
>> > implemented
>> > to deal with some of the performance regressions.
>>
>> I didn't know about the load combiner; if you have any pointers, I
>> can
>> try to look into it rather than continuing with this (less general)
>> patch then?
>
> This is not an answer to your question, but I believe that the current LoadCombiner implementation does not handle the bswap case, which you've implemented here, and would be nice to have.
>
>  -Hal

InstCombine already has code that recognizes bswap. The problem is
that it only works given values derived from an integral sized SSA
value. It doesn't work if you directly construct it from individual
byte loads. What the load combiner does is take those individual byte
loads and turn them into a single load which is then split up. The
InstCombine pass sees though all this and generates a bswap (or just
directly uses the load).

The problem comes in when all those slices aren't cleaned up, because
they are actually used independently. When this happens we actually
need to go back and split up the load.

- Michael Spencer

>
>>
>> - Ahmed
>>
>> > Right now it's in the proper place. The issue is that when we do
>> > enable load combining, this will be redundant.
>> >
>> > - Michael Spencer
>> >
>> >>
>> >> ----- Original Message -----
>> >>> From: "Ahmed Bougacha" <ahmed.bougacha at gmail.com>
>> >>> To: "ahmed bougacha" <ahmed.bougacha at gmail.com>, "david majnemer"
>> >>> <david.majnemer at gmail.com>, hfinkel at anl.gov
>> >>> Cc: "amara emerson" <amara.emerson at arm.com>,
>> >>> llvm-commits at cs.uiuc.edu
>> >>> Sent: Wednesday, October 22, 2014 1:58:47 PM
>> >>> Subject: Re: [PATCH] [InstCombine] Fold endian-independent load
>> >>> sequence into a single load.
>> >>>
>> >>> Keep the alignment from the first byte load instruction.
>> >>> Also, refactor the ByteLoads handling to make it
>> >>> endian-independent
>> >>> (from a memory standpoint).
>> >>>
>> >>> The previous patch relied on the later InstCombines to set the
>> >>> alignment, but we can also do it here. I didn't do that from the
>> >>> beginning because I couldn't think of a case where that's
>> >>> possible
>> >>> but the later InstCombine isn't. I still can't, but that's no
>> >>> reason
>> >>> not to do it here!
>> >>>
>> >>> http://reviews.llvm.org/D5898
>> >>>
>> >>> Files:
>> >>>   lib/Transforms/InstCombine/InstCombine.h
>> >>>   lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> >>>   test/Transforms/InstCombine/endian-independent-load-BE.ll
>> >>>   test/Transforms/InstCombine/endian-independent-load-LE-aliasing.ll
>> >>>   test/Transforms/InstCombine/endian-independent-load-LE.ll
>> >>>
>> >>
>> >> --
>> >> Hal Finkel
>> >> Assistant Computational Scientist
>> >> Leadership Computing Facility
>> >> Argonne National Laboratory
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory