This is an archive of the discontinued LLVM Phabricator instance.

[LV] Add flag for ignoring target info
AbandonedPublic

Authored by mssimpso on Nov 18 2016, 2:14 PM.

Details

Reviewers
mkuper
gilr
Summary

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.

Event Timeline

mssimpso updated this revision to Diff 78579.Nov 18 2016, 2:14 PM
mssimpso retitled this revision from to [LV] Add flag for ignoring target info.
mssimpso updated this object.
mssimpso added reviewers: mkuper, gilr.
mssimpso added subscribers: llvm-commits, mcrosier.
mkuper edited edge metadata.Nov 18 2016, 3:23 PM

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.

I like the idea too!

Thanks,
Michael

gilr edited edge metadata.Nov 21 2016, 7:24 AM

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.

anemet added a subscriber: anemet.Nov 22 2016, 10:11 PM

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

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"?

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.

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.

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.

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).

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.

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".

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.

mssimpso abandoned this revision.Nov 30 2016, 8:35 AM

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.