[PATCH] D122126: [LoopVectorize] Don't interleave when the number of runtime checks exceeds the threshold

[Tiehu Zhang via Phabricator via llvm-commits]

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