D30086: Add generic IR vector reductions

[Michael Kuperstein via llvm-commits]

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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170220/22d8f69c/attachment.html>