[PATCH] D132585: [VPlan] Add field to track if intrinsic should be used for call. (NFC)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.
Herald added a subscriber: pcwang-thead.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8306
 
   Intrinsic::ID ID = getVectorIntrinsicIDForCall(CI, TLI);
   if (ID && (ID == Intrinsic::assume || ID == Intrinsic::lifetime_end ||
----------------
We already get the vector intrinsic ID here, reuse it instead of getting it again repeatedly below? (Independent of patch.)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8313
 
-  auto willWiden = [&](ElementCount VF) -> bool {
-    Intrinsic::ID ID = getVectorIntrinsicIDForCall(CI, TLI);
+  auto WillWiden = [&](ElementCount VF) -> bool {
     // The following case may be scalarized depending on the VF.
----------------
`WillWiden` >> `CanUseVectorCall`? Using an intrinsic is also widening.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8317
     // version of the instruction.
     // Is it beneficial to perform intrinsic call compared to lib call?
     bool NeedToScalarize = false;
----------------
Above comment deserves an update.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8329
+        InstructionCost CallCost =
+            CM.getVectorCallCost(CI, VF, NeedToScalarize);
+        InstructionCost IntrinsicCost =
----------------
Avoid considering CallCost if NeedToScalarize is true?

Avoid getting decision and clamping Range if !ID, when a vector call can be used, e.g., w/o clamping Range (WillWiden)?

The compound decision for which (range of) VF's to use an intrinsic vs. call vs. neither should probably be retained instead of decomposing it into two independent clamps? Calls for better test coverage to make sure patch is indeed NFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132585