[PATCH] D102253: [LV] Prevent vectorization with unsupported element types.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1515
 
+  bool canVectorizeInstructionTypes(ElementCount VF) {
+    for (BasicBlock *BB : TheLoop->blocks())
----------------
david-arm wrote:
> fhahn wrote:
> > david-arm wrote:
> > > nit: Could you add a simple comment above the function before committing? Something like
> > > 
> > >   /// Returns true if all types found in the loop are legal to vectorize.
> > > 
> > > maybe?
> > I think terminology used in other places is 'widening' instead of 'vectorizing' , both in the cost model and codegen. Does vectorizing here means something different? Also would be good to add a comment for the function..
> I guess they're the same? I hadn't deliberately chosen the word `vectorize` over `widen` to be honest. In this case I think we specifically want to know if the target has hardware support for vector instructions involving a given type because we cannot fall back on scalarisation.
> 
> In general, is there a preference to using `Widen` instead of `Vectorize` in naming schemes and comments? It's just I do see other functions in this file use the word `Vectorize` in functions and class names so I guess it seemed natural to create functions with the word `Vectorize` in them.
I don't think there's a definitive answer. The current usage is not as clear cut as I thought. I was thinking about the terminology used for the various `widenInstruction`, `widenGEP`, `widenIntOrFpInduction`, `widenPHIInstruction` & co vs. `vectorizeInterleaveGroup` which does not simplify widen instructions, together with the widening decision terminology used in the cost model.

In any case, a doc-comment would definitely be helpful. Another thing to consider to align the name with `TTI.isLegalToVectorizeElementType` to something like `isLegalTo(Vectorize/Widen)InstructionType`


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

https://reviews.llvm.org/D102253