[PATCH] D96287: [TTI] Change getOperandsScalarizationOverhead to take Type args

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes! I think moving the creation of the Type array to the callers is an improvement, it makes it more clear what types are used to calculate the cost.
Just a few more nits, and then I'm happy.



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:726-727
   /// Estimate the overhead of scalarizing an instructions unique
   /// non-constant operands. The types of the arguments are ordinarily
   /// scalar, in which case the costs are multiplied with VF.
   unsigned getOperandsScalarizationOverhead(ArrayRef<const Value *> Args,
----------------
Comment needs updating.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:637
   }
 
   unsigned getScalarizationOverhead(VectorType *InTy,
----------------
nit: can you add a comment to describe the meaning of the arguments?

Just my mental understanding:
* InTy is the type of the vector(ized) node that needs scalarizing, hence it's always a VectorType.
* Args are the arguments to the node. These arguments are passed to see e.g. if any of them is a constant. If the question originates from the loopvectorizer, then the node and arguments are likely all vectorized with type InTy. Otherwise, they may have other types (such as the matrix intrinsic), but that information cannot be deduced from the arguments themselves yet, because the vectorization may not have happened yet.
* Tys are the actual types of each of the arguments after vectorizing, and are used to calculate the scalarization cost.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:647
+    if (!Args.empty()) {
+      SmallVector<Type *> Tys(Args.size(), Ty);
+      Cost += getOperandsScalarizationOverhead(Args, Tys);
----------------
dmgreen wrote:
> sdesmalen wrote:
> > dmgreen wrote:
> > > sdesmalen wrote:
> > > > Is this not doing something different than it did before, i.e. it now always uses `InTy` for each of the arguments to get the cost, where it previously used the types from each of the arguments?
> > > > I expected something similar to what you did in LoopVectorize with `MaybeVectorizeType`.
> > > Yeah. This version of getScalarizationOverhead is only called from getArithmeticInstrCost (or ARMTTIImpl::getArithmeticInstrCost etc). So the arguments/return value are all likely to all be the same value, and this is making that assumption.  It is probably at least good enough as a heuristic, when compared to the else below. I've added a comment too.
> > If this code assumes all arguments are of the same type, can you add an `assert` which checks this?
> Unfortunately that isn't something that is always true. The type of Args may be incorrect, even the scalar type. The vectorizer can do bitwidth narrowing at the same time as vectorization, in which case the Ty will be the most accurate type available. If the original Arg type was an i32, but the vectorizer is vectorizing with it as an i8 to maximize the vector factor, a vXi8 is the correct type to use. That will be Ty for all the uses of this function.
> 
> I could try to move this array creation to the callers of the function. That would involve duplicating this bit of code in a few places, but after e771614bae0a05 removed the only other use of it, it should ensure these types are correct from the outside.
Thanks for the explanation and changes!

nit: This comment ("// Assume all arguments are of type Ty, ..") can now be removed.


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

https://reviews.llvm.org/D96287