[PATCH] D75069: [LoopVectorizer] Inloop vector reductions

Mon Mar 2 05:03:00 PST 2020

dmgreen added a comment.

Thanks for taking a look!

I noticed a comment in VPValue that says:

> DESIGN PRINCIPLE: Access to the underlying IR must be strictly limited to
>  the front-end and back-end of VPlan so that the middle-end is as
>  independent as possible of the underlying IR. We grant access to the
>  underlying IR using friendship. In that way, we should be able to use VPlan
>  for multiple underlying IRs (Polly?) by providing a new VPlan front-end,
>  back-end and analysis information for the new IR.

That's the part that confused me about optimising vplans in-place. Does that mean that we have to replicate all the opcode/flags data into vplan too? Not just the use-defs. So it knows what is an Add and what is a Mul (and if they are nsw etc). What would you say counts as "middle-end" for vplan? I would presume the costmodelling and creating vplans from one another, but I didn't follow vplans early days very closely.

> I think it's best to move the TTI hook into a separate change

Can do. I have some other changes that I was trying out too about adding a VPReductionPhi. Not sure it makes sense yet though.

> Do you know how 'experimental' the reduction intrinsics still are? It would be good to move them out of experimental at some point, especially when making use of them in LV.

As far as I understand, on architectures that request it (AArch64 and Arm MVE now), we have been using the "experimental" reduction intrinsics since they were added, in the code generated after the vector body. There is a useReductionIntrinsic which the targets can override to prefer them over the IR equivalents.

I would like to add a mask to the reduction intrinsics. That will be important for getting tail-predicated inloop reductions to work. Whether this is done by adjusting the existing ones or adding new ones, I'm not sure. Obviously some architectures do not handle the mask, only handling a all-one mask. After that is sorted out then I agree it would be nice to drop the "experimental".

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75069/new/

https://reviews.llvm.org/D75069