Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[LV] Support strided load with a stride of -1 #128718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[LV] Support strided load with a stride of -1 #128718
Changes from all commits
99fe837
991267f
d967afe
a0b61ad
95a890b
4d0301a
f67a627
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strided
means negative stride? Would be good to clarify in the comment for Strided..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for now. Later it is supposed to support positive (and <-1 negative) and runtime stride values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
Strided
eventually be some sort ofstd::optional<int>
? Or is the plan to also allow runtime strided values? In which case I presume we need to model it as a VPValue operand.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the initial patch for strided memory accesses, currently only supporting cases where the stride is -1. A follow-up patch will focus on stride analysis to support more strided memory accesses.
However, I believe it would be beneficial to discuss and decide how VPlan should handle stride in this initial patch. My current plan is to extend the existing
VPWidenLoadRecipe/VPWidenStoreRecipe
andVPVectorPointerRecipe
by explicitly passing the stride into the recipe. For example:A memory access will only be considered strided when
Stride
is explicitly provided (i.e., not nullptr).@fhahn @ayalz What do you think about this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @fhahn @ayalz. Do you think this is a good approach? Or any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add another 'mode' to WidenLoad/StoreRecipe to make it even more complicated.
It would probably be good to have a separate recipe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to support it for EVL tail folding further down the line would we also need to add an EVL + Strided load recipe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fhahn I see. Let's first try adding VPStridedLoadRecipe for strided loads. What about VPVectorPointerRecipe? Can we modify it to:
Or should we introduce a new recipe instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukel97 I think we can pass VF into the new recipe, and replace VF with EVL, like VPVectorEndPointerRecipe did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mel-Chen yes, that's a good idea. Since it will generate a vp intrinsic either way.