[PATCH] D59995: [LV] Exclude loop-invariant inputs from scalar cost computation.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Some format typos, and clarifying if needsExtract() should assume vectorized or scalarized before scalars are computed.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1180
+  /// needed. The flag NeedToScalarize shows if the call needs to be scalarized -
+  // i.e. either vector version isn't available, or is too expensive.
+  unsigned getVectorCallCost(CallInst *CI, unsigned VF,
----------------
`// >> ///` on last line too ;-)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1182
+  unsigned getVectorCallCost(CallInst *CI, unsigned VF,
+                                  bool &NeedToScalarize);
+
----------------
indentation?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1343
+    if (VF > 1 && Scalars.find(VF) == Scalars.end())
+      return true;
+
----------------
Ahh, is it better to assume we can vectorize V, contrary to the above comment?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3151
+  // Skip operands that do not require extraction/scalarization and do not incur
+  // any overhed
+  return TTI.getIntrinsicInstrCost(
----------------
overhed >> overhead.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4130
+    unsigned CallCost =
+        Cost->getVectorCallCost(CI, VF, NeedToScalarize);
     bool UseVectorIntrinsic =
----------------
one line?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5669
 
+unsigned LoopVectorizationCostModel::getScalarizationOverhead(Instruction *I, unsigned VF) {
+
----------------
two lines?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5686
+  // Collect operands to consider.
+  Instruction::op_range Ops(I->op_begin(), I->op_begin());
+  if (CallInst *CI = dyn_cast<CallInst>(I))
----------------
Effectively initializing Ops to an empty range by default? This is an orthogonal refactoring; consider instead having another early-exit:
```
if (isa<StoreInst>(I) && TTI.supportsEfficientVectorElementLoadStore())
  return Cost;
```
and initializing Ops to I->operands() by default.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6664
+      unsigned CallCost =
+          CM.getVectorCallCost(CI, VF, NeedToScalarize);
       bool UseVectorIntrinsic =
----------------
one line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59995