[PATCH] D97861: [LoopVectorize][NFC] Refactor code to use IRBuilder::CreateStepVector

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2265
+
+  // Add on StartIdx
+  Value *StartIdxSplat = Builder.CreateVectorSplat(
----------------
ctetreau wrote:
> It seems like this does the same thing as the original version. However, I don't think this function does what it claims to do.
> 
> The docs say that this computes `Val + <StartIdx, StartIdx + Step, StartIdx + 2*Step, ...>`
> 
> But what this function does is:
> ```
> StartIdxSplat = <StartIdx, StartIdx, ...>
> InitVec = StartIdxSplat + <0, 1, ...>
> Step = <InputStep, InputStep, ...>
> result = Val + (InitVec * Step)
> ```
> 
> If the input step is 1, this seems to do the right thing:
> 
> ```
> StartIdx = 2 // some non-1 start for illustrative purposes
> Val = <a, b, c, d>
> InputStep = 1
> 
> StartIdxSplat = <2, 2, 2, 2>
> InitVec = <2, 2, 2, 2> + <0, 1, 2, 3> = <2, 3, 4, 5>
> Step = <1, 1, 1, 1>
> result = <a, b, c, d> + (<2, 3, 4, 5> * <1, 1, 1, 1>) = <a, b, c, d> + <2, 3, 4, 5>
> ```
> 
> But if we try a larger input step:
> 
> ```
> StartIdx = 2 // some non-1 start for illustrative purposes
> Val = <a, b, c, d>
> InputStep = 2
> 
> StartIdxSplat = <2, 2, 2, 2>
> InitVec = <2, 2, 2, 2> + <0, 1, 2, 3> = <2, 3, 4, 5>
> Step = <2, 2, 2, 2>
> result = <a, b, c, d> + (<2, 3, 4, 5> * <2, 2, 2, 2>) = <a, b, c, d> + <4, 6, 8, 10>
> ```
> 
> The label on the tin says that the first element of the RHS vector should be equal to `StartIdx`, which it clearly isn't. I haven't scrutinized the floating point codepath, but I assume it has a similar issue.
> 
> I believe we need to multiply the step vector by the step before adding the start value.
> 
> I feel like we should do something about this. Either assert that the step is 1 and add a fixme, or just fix the function.
I suppose another option is to document what the function actually does in the header


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

https://reviews.llvm.org/D97861