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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits