Page MenuHomePhabricator

[NFC][LoopVectorize] Change getStepVector to take a Value* for the StartIdx
ClosedPublic

Authored by david-arm on Oct 15 2021, 5:29 AM.

Details

Summary

This patch changes the definition of getStepVector from:

Value *getStepVector(Value *Val, int StartIdx, Value *Step, ...

to

Value *getStepVector(Value *Val, Value *StartIdx, Value *Step, ...

because:

  1. it seems inconsistent to pass some values as Value* and some as integer, and
  2. future work will require the StartIdx to be an expression made up of runtime calculations of the VF.

In widenIntOrFpInduction I've changed the code to pass in the
value returned from getRuntimeVF, but the presence of the assert:

assert(!VF.isScalable() && "scalable vectors not yet supported.");

means that currently this code path is only exercised for fixed-width
VFs and so the patch is still NFC.

Diff Detail

Event Timeline

david-arm created this revision.Oct 15 2021, 5:29 AM
david-arm requested review of this revision.Oct 15 2021, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2021, 5:29 AM
david-arm updated this revision to Diff 380970.Oct 20 2021, 8:40 AM
  • Refactored the patch a little to avoid relying on getRuntimeVF handling FP types.
kmclaughlin added inline comments.Oct 27 2021, 2:58 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1122

Hi @david-arm, could this be merged with getRuntimeVF? Where we create the sitofp if the type passed to the function is a floating point type?

david-arm marked an inline comment as done.Oct 27 2021, 3:31 AM
david-arm added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1122

Hi @kmclaughlin, that's a good question! I did this originally with D111873, but one of the reviewers left a comment suggesting it seemed an unusual thing to do.

kmclaughlin accepted this revision.Oct 27 2021, 3:49 AM
This revision is now accepted and ready to land.Oct 27 2021, 3:49 AM
fhahn added inline comments.Oct 27 2021, 3:53 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1122

nit: could this be static? and outside the llvm namespace?

david-arm marked an inline comment as done.Oct 27 2021, 3:59 AM
david-arm added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1122

Hi @fhahn, I can change this to static easily enough before merging if that's ok? However, for changing the namespace should that be in a separate follow-on patch? The problem is that getRuntimeVFAsFloat calls getRuntimeVF, which itself is used outside of this file. It would be a bit strange to have one outside the llvm namespace and one inside I think?

fhahn accepted this revision.Oct 27 2021, 7:15 AM

LGTM, thanks

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1122

Adding the static on the committed version sounds good, thanks!

With respect to moving that also sounds good as potential Followup if it is not straight forward due to the depency. Probably not worth worrying once it is static.