D30086: Add generic IR vector reductions

Tue Feb 21 01:32:27 PST 2017

That sounds reasonable. The late lowering fallback solution is
something that I wanted to work towards in the long term anyway.

Amara

On 21 February 2017 at 07:38, Michael Kuperstein via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> I don't disagree with you - I think the general consensus is that we want
> target-independent representations for vector idioms, instead of generating
> target-dependent code in LV.
>
> So, if we can't find a generic representation people are happy with, we're
> in trouble. But my understanding was that this representation *is* something
> the relevant backends are happy with. If that's the case, why do we want an
> interface that would create it conditionally? That's fine if this is
> experimental (in which case it should say so), and those are interfaces
> we'll end up removing in favor of late lowering when we decide the intrinsic
> is stable. But I don't think we want an optional intrinsic as the steady
> state, unless there's a really compelling reason.
>
> One such compelling reason would be "this badly breaks optimizations of
> shuffles", but if it does, then we probably have a problem on our hands
> regardless.
>
> On Mon, Feb 20, 2017 at 11:18 PM, Demikhovsky, Elena
> <elena.demikhovsky at intel.com> wrote:
>>
>> > If we know this is not the right generic representation, this should not
>> > go in as a target-independent intrinsic. I haven't seen anyone raise any
>> > "deal-breaker" issues so far, though.
>> My question is more general, not really related to this review. Loop
>> Vectorizer is a common engine, we don't have any target specific plug-in
>> except TTI. How do you see using target specific intrinsics inside the
>> vectorizer? And it's not only Builder.CreateXXX(), it's a sequence that
>> should be recognized that may end up with a  different loop form, or loop
>> tail.
>> I think that we all are looking for a way to define target specific staff
>> as a common, since the Vectorizer does not have an infrastructure for target
>> specific algorithms.
>>
>> -  Elena
>>
>> -----Original Message-----
>> From: Michael Kuperstein via Phabricator [mailto:reviews at reviews.llvm.org]
>> Sent: Tuesday, February 21, 2017 01:02
>> To: amara.emerson at arm.com; renato.golin at linaro.org; Saito, Hideki
>> <hideki.saito at intel.com>; james.molloy at arm.com; llvm-dev at redking.me.uk;
>> a.bataev at hotmail.com; mkuper at google.com; Demikhovsky, Elena
>> <elena.demikhovsky at intel.com>; mehdi.amini at apple.com
>> Cc: schuett at gmail.com; mssimpso at codeaurora.org; mzolotukhin at apple.com;
>> llvm-commits at lists.llvm.org
>> Subject: [PATCH] D30086: Add generic IR vector reductions
>>
>> mkuper added a comment.
>>
>> In https://reviews.llvm.org/D30086#681447, @aemerson wrote:
>>
>> > In https://reviews.llvm.org/D30086#680635, @mkuper wrote:
>> >
>> > > I can see some advantages to forming the intrinsic conditionally, but
>> > > I'm not sure they're worth the cost of not having a canonical
>> > > representation.
>> >
>> >
>> > This patch was intended to be a transitional stage, my expectation was
>> > that over time targets would decide to move to this representation by opting
>> > in and handling them. The reason for the transitional step was that I didn't
>> > want to force this form on all targets (even if it lowers to the same code
>> > by a lowering step) as it changes the IR during the mid-end stage. At the
>> > moment, the only target which requires this intrinsic form without any
>> > alternative is SVE. If you think it's ok to change the reduction form for
>> > all targets until a late lowering then I'm happy to implement that.
>>
>>
>> What I'd really like to avoid is having a nominally "target-independent"
>> intrinsic, which can, in practice, only be handled by one or two targets.
>>
>> There are three ways I can see this going:
>> a) If we know this is not the right generic representation, this should
>> not go in as a target-independent intrinsic. I haven't seen anyone raise any
>> "deal-breaker" issues so far, though.
>> b) If we think this is the right generic representation, but we're not
>> sure, we can introduce these intrinsics as llvm.experimental. For an
>> experimental intrinsic, I think it makes more sense to have targets opt-in
>> like this patch does (but we probably still want default lowering for
>> targets that don't opt-in).
>> c) If we are sure about this, then we should do just it for all targets,
>> and have late lowering by default. I'm not really the right person to
>> determine that, though. I guess that's a question for the backend owners.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D30086
>>
>>
>>
>> ---------------------------------------------------------------------
>> Intel Israel (74) Limited
>>
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>