This patch adds a flag ("loop-vectorize-ignore-target-info") that causes the vectorizer to use the default TTI implementation instead of one based on the target. This is useful for ensuring consistent results across targets when testing. The run lines of all target-independent tests have been updated to use the flag.
Diff Detail
- Build Status
Buildable 1426 Build 1426: arc lint + arc unit
Event Timeline
I like this idea, but that's not very surprising since I suggested it. :-)
So another sanity check would be nice.
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
2092 | Maybe it'll look a bit simpler if we make TTI a reference? E.g. something like (haven't checked if it compiles :-) ) TargetTransformInfo BaseTTI = TargetTransformInfo(F.getParent()->getDataLayout()); TargetTransformInfo &TTI = BaseTTI; if (!IgnoreTargetInfo) TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F) | |
7454 | Please also update this to honor IgnoreTargetInfo. |
Matt, this patch also seems to solve the problem we had with D25333 of how to write non-target-specific cost-model tests. Is that correct?
If so, maybe also move the test added back then out of AArch64? (There are also some other tests I noticed back then currently duplicated for AArch/ARM and X86 that might also be movable to the platform-independent zone).
Thanks for all the feedback, all. Just a heads up that Matt will be out of the office until the 28th due to the holidays.
Thanks for the feedback everyone! I'll incorporate the suggested changes and update the patch after the holidays.
Hi Matt,
You should probably circulate this more widely on llvm-dev. This is a new policy on how one should write tests going under test/Transform/LoopVectorize.
Thanks,
Adam
It's more of a bugfix in the current policy - we used to have "target independent" tests because providing a manual UF and VF would, in effect, mean we'd make no cost model queries that affect the output. That was basically dumb luck - conceptually, tests for passes that rely on TTI need to either specify a target, or somehow be forced to be target-independent.
But I agree, it may be worth circulating more widely, since this problem probably exists outside the LV vectorizer, so we may want a more principled way to do it.
Do you have an opinion on the concept itself, other than "circulate more widely"?
Also there is actual shift of how tests should be written now. I think that we all got used to writing these tests by forcing the vectorization and interleave factors.
Do you have an opinion on the concept itself, other than "circulate more widely"?
I haven't been following the review closely where this came up so I didn't understand the rational and the summary does not really explain how the current practice breaks down. I am assuming we want to write target-indepedent tests but the vectorizer needs a cost model.
Why aren't we adding a forcing flag for this feature as well, just like forcing the vectorization/interleave factors? That may make the test more explicit rather than using the default TTI. To me that would be the generalization of the current concept.
I guess what I am missing to get convinced is a run-down of the alternatives. I know this was discussed in the other review but there is no reference here. It's good to document these things for posterity.
I think most tests will still want to provide explicit vectorization and interleave factors, because we usually want to force vectorization with a specific factor, as opposed to "do the default thing, whatever it happens to be".
Do you have an opinion on the concept itself, other than "circulate more widely"?
I haven't been following the review closely where this came up so I didn't understand the rational and the summary does not really explain how the current practice breaks down. I am assuming we want to write target-indepedent tests but the vectorizer needs a cost model.
The gist is that we now query the cost model to make predication decisions, not just determine the VF and UF.
Why aren't we adding a forcing flag for this feature as well, just like forcing the vectorization/interleave factors? That may make the test more explicit rather than using the default TTI. To me that would be the generalization of the current concept.
This is exactly why I asked for additional opinions. IIRC this is what Matt originally suggested.
What I'm afraid of is flag proliferation - I wouldn't want to have to add another flag to all "target-independent" tests each time we add a cost model query that affects the way the vectorized code looks (as opposed to affecting the VF and UF).
Sure I mean that there is a new option (-loop-vectorize-ignore-target-info) that probably most new tests need to add.
Do you have an opinion on the concept itself, other than "circulate more widely"?
I haven't been following the review closely where this came up so I didn't understand the rational and the summary does not really explain how the current practice breaks down. I am assuming we want to write target-indepedent tests but the vectorizer needs a cost model.
The gist is that we now query the cost model to make predication decisions, not just determine the VF and UF.
Why aren't we adding a forcing flag for this feature as well, just like forcing the vectorization/interleave factors? That may make the test more explicit rather than using the default TTI. To me that would be the generalization of the current concept.
This is exactly why I asked for additional opinions. IIRC this is what Matt originally suggested.
What I'm afraid of is flag proliferation - I wouldn't want to have to add another flag to all "target-independent" tests each time we add a cost model query that affects the way the vectorized code looks (as opposed to affecting the VF and UF).
OK, so let's continue discussing this on llvm-dev.
Thanks again for the feedback everyone! I'll start the thread on llvm-dev, and we can continue there.
Based on Adam's comment from llvm-dev, it looks like we already are using the default TTI when we don't specify a target. So this patch is not needed after all. Thanks everyone for the feedback.
Maybe it'll look a bit simpler if we make TTI a reference?
E.g. something like (haven't checked if it compiles :-) )