[PATCH] D137913: [X86] Rewrite `getScalarizationOverhead()`

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:4470
+            if (!AffectedLanes[I] ||
+                (Lane == 0 && FullyAffectedLegalVectors[LegalVec]))
+              continue;
----------------
lebedev.ri wrote:
> vdmitrie wrote:
> > I just came across this patch and I'd like to ask some clarification question as I'd like to better understand the intent.
> > A quick remark before the question. In vectorizer term "lane" is typically mapped into individual vector element while when referencing memory/cache that is another thing. So in order to avoid confusion I will be referencing vector element explicitly and use "sub-vector" instead of "Lane" here.
> > 
> > Lets consider an example where we have 16 elements in of 32bits type each, like <16 x f32 >.
> > On avx512 architecture that will be just single legal vector which consists of four sub-vectors of < 4 x f32 > types.
> > Now consider two cases:
> > 1) we are building a full vector inserting each element of the legal vector
> > 2) we are inserting some of elements in every sub-vector in the legal vector (let it be each odd element for simplicity)
> > 
> > in the case 1, as we are building the entire vector, we don't have any upper elements to preserve so we don't really need insert there. So I assume that this is the case that supposed to be targeted (to be precise this should be not the entire vector build but rather all the upper sub-vectors build)
> > 
> > in case 2 we have to preserve some elements of upper sub-vectors. If I'm not mistaken It means that we still need to use insert for lowest sub-vector as well in order to preserve these elements (the upper part of the legal vector).
> > But as it done in this patch: AffectedLanes will be {1,1,1,1} as each sub-vector has a couple of elements to be inserted (preserving the other two). Derived form that FullyAffectedLegalVector[0] will also be '1' in that case and we will not account for  insert of the lowest sub-vector. Is than done intentionally or this is just a mistake?
> > Should we instead to check that we have nothing to preserve in any of the upper sub-vectors?
> > 
> > I just came across this patch and I'd like to ask some clarification question as I'd like to better understand the intent.
> 
> > A quick remark before the question. In vectorizer term "lane" is typically mapped into individual vector element while when referencing memory/cache that is another thing. So in order to avoid confusion I will be referencing vector element explicitly and use "sub-vector" instead of "Lane" here.
> >
> > Lets consider an example where we have 16 elements in of 32bits type each, like <16 x f32 >.
> On avx512 architecture that will be just single legal vector which consists of four sub-vectors of < 4 x f32 > types.
> > Now consider two cases:
> >
> > 1. we are building a full vector inserting each element of the legal vector
> > 2. we are inserting some of elements in every sub-vector in the legal vector (let it be each odd element for simplicity)
> 
> (How to boil water: pour water into kettle, boil.
> how to boil water in half-full kettle: remove water, and see original recipe.)
> 
> These two cases are equivalent, or i do not understand the question.
> Note that all of the logic here operates on XMM vectors.
> 
> As long as XMM subvector is touched by an element insertion,
> and not elements are overridden, we need to first extract said subvector,
> insert into it, and then insert the subvector back
> into the original legal vector.
> 
> In case 2., you said that every XMM subvector receives some elements,
> so what subvector would we need to preserve, if all of them are affected?
@vdmitrie I'm struggling to follow to your description - maybe it would be easier if you create a godbolt link with code examples please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137913