[PATCH] D109368: [LV] Don't vectorize if we can prove RT + vector cost >= scalar cost.

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:449
+                      ProfileSummaryInfo *PSI, GeneratedRTChecks &RTChecks,
+                      ElementCount MinProfTripCount)
+      : OrigLoop(OrigLoop), PSE(PSE), LI(LI), DT(DT), TLI(TLI), TTI(TTI),
----------------
Since 'minimum profitable trip count' is part of VectorizationFactor and InnerLoopVectorizer should know both VecWidth and MinProfTripCount I would suggest passing that information as single VectorizationFactor argument.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8240
+    // Now we can compute the minimum required trip count TC as
+    //   (RtC + EpiC) / (ScalarC + (VecC / VF)) < TC
+    //
----------------
The expression 'VecC imul (TC idiv VF)' is not associative since it involves an integer division (idiv). Thus it's not legal to simply replace it with 'TC fmul (VecC fdiv VF)'. Maximum possible error is up to 'VF'. In other words 'TC fmul (VecC fdiv VF)' - 'VecC imul (TC idiv VF)' <= VF. That means 'MinTC1' computed that way is an upper estimate of actual minimum. I don't see anything terribly bad in taking upper estimate but IMHO worth mentioning in comments.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8263
+    SelectedVF.MinProfTripCount =
+        ElementCount::getFixed(MinTCDown == MinTC ? MinTC : MinTCDown + VF);
+
----------------
Please use alignTo() instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109368/new/

https://reviews.llvm.org/D109368