[PATCH] D89322: [LV] Initial VPlan cost modelling

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:195
+  /// The VF/Cost from costing
+  VectorizationFactor VF;
+};
----------------
This API seems a bit weird to me. I'd expect code generation decisions to be an entity on its own, not some pair (which would very well used `std::pair` typedef).

Today is the VF we're looking at, perhaps one day we'll want to look at UF costs (less branches), particular options of the plans themselves (split/reorder outer-loops in different ways), etc.

So, for now, if it's just a return value wrapper, we can do with `std::pair` or `std::tuple` and use `auto [vplan, VF] = ...` to extract on call.

For later, if we want to carry more info without passing them all as arguments, we should have an actual `VPlanResult` struct or something, with a back pointer to the VPlan and the parameter selection.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7127
+    for (VPlanPtr &Plan : VPlans)
+      if (Plan->hasVF(VF.Width))
+        return VPlanVFPair{&*Plan, VF};
----------------
So, whatever is the first VPlan with a VF we return? Is it guaranteed to be 1?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:97
+/// vectorization has actually taken place).
+using VectorizationCostTy = std::pair<unsigned, bool>;
+
----------------
Funny enough, here the use of `std::pair` is worse. In the code that uses it, there's a lot of:
```
Cost.first += C.first;
Cost.second |= C.second;
```
and the typedef offers no help.

Here an actual struct would be more suitable:
```
struct VectoriztionCostTy {
  unsigned cost;
  bool active;
}
```


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

https://reviews.llvm.org/D89322