[PATCH] D122126: [LoopVectorize] Don't interleave when the number of runtime checks exceeds the threshold
Tiehu Zhang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 12 06:54:52 PDT 2022
TiehuZhang added a comment.
The code has been updated since accept. Please review it again. Thank you very much! @fhahn @dmgreen
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10460
IC = CM.selectInterleaveCount(VF.Width, *VF.Cost.getValue());
+ if (!UserIC && requiresTooManyRtChecks) {
+ ORE->emit([&]() {
----------------
fhahn wrote:
> Can the handling be merged into a single check & diagnostic?
Hi,@fhahn, thanks for your reply! Does the current version meet the requirements?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10460
IC = CM.selectInterleaveCount(VF.Width, *VF.Cost.getValue());
+ if (!UserIC && requiresTooManyRtChecks) {
+ ORE->emit([&]() {
----------------
TiehuZhang wrote:
> fhahn wrote:
> > Can the handling be merged into a single check & diagnostic?
> Hi,@fhahn, thanks for your reply! Does the current version meet the requirements?
Hi, @fhahn, is there any other problem with this patch?
ping
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10461
+ if (!LVP.hasTooManyRuntimeChecks()) {
+ IC = CM.selectInterleaveCount(VF.Width, *VF.Cost.getValue());
+ }
----------------
fhahn wrote:
> This check here should be sufficient, there should be no need to also check in `selectInterleaveCount`.
>
> Could you just move the remark generation & early exit from `::plan` here?
>
> You might want to skip those checks if there's a UserVF or UserIC used, with those I think we should always vectorize if possible. It also might be good to add a check line to your test which forces an interleave count > 1.
Hi, @fhahn, thanks for your review! It sounds similar to doesNotMeet (https://reviews.llvm.org/D98634), but the difference is that I need to use UserIC and UserVF to control whether this check needs to be performed, right? E.g.
```
if (!UserVF && LVP.requiresTooManyRuntimeChecks()) {
/*generate remarks*/
VF = VectorizationFactor::Disabled();
}
if (!UserIC && LVP.requiresTooManyRuntimeChecks()) {
/*generate remarks*/
IC = 1;
}
```
> Could you just move the remark generation & early exit from ::plan here?
>
> You might want to skip those checks if there's a UserVF or UserIC used, with those I think we should always vectorize if possible. It also might be good to add a check line to your test which forces an interleave count > 1.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7673-7674
// Check if it is profitable to vectorize with runtime checks.
- unsigned NumRuntimePointerChecks = Requirements.getNumRuntimePointerChecks();
- if (SelectedVF.Width.getKnownMinValue() > 1 && NumRuntimePointerChecks) {
- bool PragmaThresholdReached =
- NumRuntimePointerChecks > PragmaVectorizeMemoryCheckThreshold;
- bool ThresholdReached =
- NumRuntimePointerChecks > VectorizerParams::RuntimeMemoryCheckThreshold;
- if ((ThresholdReached && !Hints.allowReordering()) ||
- PragmaThresholdReached) {
+ if (SelectedVF.Width.getKnownMinValue() > 1) {
+ if (hasTooManyRuntimeChecks()) {
ORE->emit([&]() {
----------------
dmgreen wrote:
> Maybe just use a single if now: `if (SelectedVF.Width.getKnownMinValue() > 1 && hasTooManyRuntimeChecks()) {`
Done. Thanks for your review!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122126/new/
https://reviews.llvm.org/D122126
More information about the llvm-commits
mailing list