-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[LV] Don't trigger legacy/vplan assert when forcing costs #156870
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
Open
david-arm
wants to merge
1
commit into
llvm:main
Choose a base branch
from
david-arm:fix_force_insn_cost
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+171
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Hmm, I am wondering if the treatment of the legacy cost model makes more sense here. We could easily match it in the VPlan cost model I think, we know how many source instructions we had (# of members in the group) and could just multiply this by the forced cost?
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.
Hmm, but then I'd suggest we need a separate flag along the lines of
-force-target-recipe-cost=1so that users could really force the cost to 1 if they desire. Then this new flag would face the same issue.To be honest, even if with your suggestion I still think it doesn't make sense to assert that one completely arbitrary cost model matches another completely arbitrary cost model. In this case it feels like the assert is actually getting in the way of diligent testing of all code paths in the vectoriser.
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.
Hmm, it depends I guess, both cost models still try to compute the costs of the same result.
The difference is to either consider the cost of all instructions together in an interleave group as the the forced cost (what VPInterleaveRecipe does currently) or ignore the fact that members of the group will be combined and used the forced cost for each member.
I think one could argue for either behavior (VPInterleaveRecipe will generate a single memory op, but it is very wide and in practice we should end up with one legal memory op per member); it just would be preferable to have a consistent meaning for the flag.
I've not looked into it yet, but I think we could also match the behavior of VPInterleaveRecipe in the legacy cost model, by checking if the instruction should get lowered to an interleave group, and if so only apply the forced cost to the insertion point member.
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.
OK, I'm happy to look into that approach, but still it feels like a lot of effort to fix something that the developer probably doesn't care about. Obviously this is just my own example, but when I used the flag
-force-instruction-cost=1I wasn't interested in knowing whether they agreed or not. What I really cared about was that all code paths have been tested, nothing crashes during compilation or execution. Sometimes the assert just gets in the way of making sure a patch works correctly and I frequently have to delete the assert and rebuild.One alternative I'd also be happy with is an override flag that allows developers to disable the assert.
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.
Yeah it looks like there are at least a few other isses with forcing target instruction costs, but it seems like some of them seem more like bugs in the VPlan-based cost model. With fhahn@b781919, I can't see any crashes with
-force-target-instruction-cost=1on a quite large test set.Might be worth a try on the test set on your end, if you have a chance
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.
Not sure if you had a chance to look into this further yet, but both #168270 & #168269 should address most of the crashes. I found another one, for which I'll open a PR soon
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.
No sorry I didn't look into it, because honestly speaking I wasn't sure in the value of making the cost models match (and hence making the code more complex) when forcing the target instruction cost. It felt like a lot of effort for something that users/developers probably don't care about.
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.
The main benefit I see s that we can still catch cases where the VPlan-based cost model doesn't handle forced costs directly, as in #168372 & #168269
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.
OK I can see that point, but I had the impression the assert is something that we should be killing off soon anyway given that we're using vplan for most costs now. I guess it sounds like in all likelihood the assert is here to stay for quite a long time.
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.
Not necessarily, but making sure we handle forced costs somewhat consistently and correctly is something that should be fixed before we remove the assert I think