Page MenuHomePhabricator

[CostModel][AArch64] Make loads/stores of <vscale x 1 x eltty> expensive.
ClosedPublic

Authored by sdesmalen on Jun 8 2021, 3:25 AM.

Details

Summary

At the moment, <vscale x 1 x eltty> are not yet fully handled by the
code-generator, so to avoid vectorizing loops with that VF, we set the
cost-model to some high (expensive) value for now. The reason for not
adding a new "TTI::getMinimumScalableVF" is because the type is supposed
to be a type that can be legalized. It partially is, although the support
for these types need some more work.

Diff Detail

Event Timeline

sdesmalen created this revision.Jun 8 2021, 3:25 AM
sdesmalen requested review of this revision.Jun 8 2021, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 3:25 AM
paulwalker-arm accepted this revision.Jun 8 2021, 9:28 AM

Personally, for the testing I'd pick a different element type than i128 given that is already problematic.

This revision is now accepted and ready to land.Jun 8 2021, 9:28 AM
dmgreen added a subscriber: dmgreen.Jun 8 2021, 9:31 AM

Why 9999? Why not an invalid cost? I thought that was what invalid was for, so we didn't need hacky random numbers like this.

Why 9999? Why not an invalid cost? I thought that was what invalid was for, so we didn't need hacky random numbers like this.

We've chosen not to conflate legalization and cost-modelling, so that the cost-model must return a valid cost when the legalization phase says that a given VF is legal to use. The benefit of that is that it guards the requirement to have a complete (albeit not necessarily accurate) cost-model. For SVE a VF of vscale x 1 is legal in principle, meaning that we should be able to code-generate it. But because our code-generator is incomplete, we want to avoid using these VFs. I haven't added an interface to query the minimum legal VF, since I know we'll remove that interface at some point. The LV will expect all costs calculated to be Valid and will fail this assertion otherwise. So basically return <magic number> is just a temporary stop-gap to avoid failing that (and other) assertion failures.

Does that make more sense?

Matt added a subscriber: Matt.Jun 8 2021, 4:51 PM
sdesmalen updated this revision to Diff 350794.Jun 9 2021, 12:06 AM
  • Updated tests to use <vscale x 1 x i64> instead of <vscale x 1 x i128>

Why 9999? Why not an invalid cost? I thought that was what invalid was for, so we didn't need hacky random numbers like this.

We've chosen not to conflate legalization and cost-modelling, so that the cost-model must return a valid cost when the legalization phase says that a given VF is legal to use. The benefit of that is that it guards the requirement to have a complete (albeit not necessarily accurate) cost-model. For SVE a VF of vscale x 1 is legal in principle, meaning that we should be able to code-generate it. But because our code-generator is incomplete, we want to avoid using these VFs. I haven't added an interface to query the minimum legal VF, since I know we'll remove that interface at some point. The LV will expect all costs calculated to be Valid and will fail this assertion otherwise. So basically return <magic number> is just a temporary stop-gap to avoid failing that (and other) assertion failures.

Does that make more sense?

I would expect the vectorizer, when encountering an invalid cost, to not pick that vector factor. Not crash. Otherwise I'm not sure I understand what an invalid cost means or is for?

I'm not sure I'm a huge fan of the proliferation of isLegal.. methods. They feel poorly defined and we have two places to return one bit of information. But I can see the advantage in ruling out large swaths of code, and stopping vectorization early when we already know it won't be valid, before having to do any expensive vplan construction. I feel we should still have invalid costs blocking vectorization at those factors though, even if it does get as far as the costmodel.

My feeling is that the vectoriser should never try to vectorise a loop that is impossible to vectorise, with the act of costing a vectorisation candidate effectively saying the loop is safe to vectorise. So a way to guard this is to ensure that when costing a vectorisation candidate we force the need for a real cost. It's just unfortunate that when it comes to scalable vectorisation there are more scenarios that lead to loops that are not vectorisable (or rather, loops where scalable VFs must be pulled from the list of candidate VFs).

Now I agree that with this patch we're not exactly following this design goal but for SVE this is currently an experimental (default off) feature and thus we have wiggle room to do the wrong thing for the right reasons. In this instance it is more valuable to allow a base level of experimental vectorisation before all the code generation bugs are resolved. As Sander said, we could fix this by honouring the design (i.e. as part of the is legal checks) but that in itself would be a redundant interface once the code generation bug is fixed.

SjoerdMeijer added a comment.EditedJun 9 2021, 6:07 AM

Why 9999? Why not an invalid cost? I thought that was what invalid was for, so we didn't need hacky random numbers like this.

We've chosen not to conflate legalization and cost-modelling, so that the cost-model must return a valid cost when the legalization phase says that a given VF is legal to use. The benefit of that is that it guards the requirement to have a complete (albeit not necessarily accurate) cost-model. For SVE a VF of vscale x 1 is legal in principle, meaning that we should be able to code-generate it. But because our code-generator is incomplete, we want to avoid using these VFs. I haven't added an interface to query the minimum legal VF, since I know we'll remove that interface at some point. The LV will expect all costs calculated to be Valid and will fail this assertion otherwise. So basically return <magic number> is just a temporary stop-gap to avoid failing that (and other) assertion failures.

Does that make more sense?

I think I understand that, but I am afraid I am still with Dave as return 9999 looks so extremely hacky.
I.e., I understand return <magic number> is a stop-gap, but why can we not use return Invalid instead as a stop-gap? I do see the point about conflating legalization and cost-modelling, but I don't that is really important here as we are talking about plugging a hole, not about a decision decision that we want to keep. Thus, I don't think we would be conflating things if we return Invalid here, if that works, and I would also vote for that. The comments where return 9999 is done has already the required explanations what is going on.

It seems like a simpler overall route to remove the assert from the vectorizer, and allow it to deal with invalid costs. But I am not blocking this patch.

(Although, you might want to pick a number higher than 9999, it's not particularly high. And like Sjoerd suggested giving it a name might be helpful).

It seems like a simpler overall route to remove the assert from the vectorizer, and allow it to deal with invalid costs. But I am not blocking this patch.

(Although, you might want to pick a number higher than 9999, it's not particularly high. And like Sjoerd suggested giving it a name might be helpful).

Agreed with everything what Dave said.
Removing the assert for invalid costs seems easiest to me. And I am also not blocking this, but if you want to go ahead with the 9999 we should at least define and use a constant

CM_SVE_EXPENSIVE_HACK

or something along the lines and document what the idea and the plan is (if that isn't there already somewhere).

sdesmalen updated this revision to Diff 355202.Jun 29 2021, 6:12 AM

Rebased onto D105108 to use InstructionCost::getMax() (instead of a random number)

sdesmalen updated this revision to Diff 355210.Jun 29 2021, 6:24 AM
  • Removed unnecessary whitespace (apologies for the noise)

As far as I understand, the backend currently doesn't handle <vscale x 1 x ...> types? Is "invalid" not a better way to represent that, than "very expensive"? Even if it is only until the backend is fixed.

sdesmalen updated this revision to Diff 357906.Mon, Jul 12, 5:13 AM

Use getInvalid() instead of getMax().

The downside is that this will increase the number of instructions printed
in the opt-remark (D105806) but I agree that it seems more correct to
mark them as Invalid, since we can't safely code-generate these types
at the moment.

dmgreen accepted this revision.Mon, Jul 12, 10:01 AM

Thanks. LGTM.

This revision was landed with ongoing or failed builds.Wed, Jul 14, 8:44 AM
This revision was automatically updated to reflect the committed changes.