[PATCH] D149842: Scalarizer: limit scalarization for small element types

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/Scalarizer.cpp:66
 
+static cl::opt<unsigned> ClScalarizeMinBits(
+    "scalarize-min-bits", cl::init(0), cl::Hidden,
----------------
I think I misunderstood this at first.

My interpretation was that if setting this to 16 it would not scalarize vectors with element sizes up to 16 bits.
So it wouldn't scalarize `<16 x i8>` or `<4 x i16>` while it would scalarize `<2 x i24>` and `<2 x i32>`.

But this size is not mapping to the element sizes, right?
We could not get some kind of vector split/re-partition from `<16 x i8>` to `<2 x i8>`.  So it is not really scalarizing as the value still will be a vector.

Not sure exactly how to rephrase it to make that clearer (considering that I misunderstood this to be an element size).

Maybe I got fooled by the slogan for this patch "limit scalarization for small element types". I actually thought that I would see something that prevented scalarization from happening when the //element size// was smaller than a threshold. But what the patch actually seem to do is to prevent scalarization to happen //for large vector factors// (it just splits it up into using smaller vectors instead of scalarizing).

So everywhere in this pass when it says "scalarize" I guess one should read it as "split" (or "resize" or something similar). For example code comments saying "Perform actual scalarization" could be followed by code that emit vector operations.


================
Comment at: llvm/lib/Transforms/Scalar/Scalarizer.cpp:204
+  // Return the alignment of fragment I.
+  Align getFragmentAlign(unsigned I) {
+    return commonAlignment(VecAlign, I * SplitSize);
----------------
nit: You've changed `I` to `Frag` in other places where we no longer refer to a vector index. I think it doesn't hurt to do it here as well for consistency.


================
Comment at: llvm/lib/Transforms/Scalar/Scalarizer.cpp:390
 
 // Return component I, creating a new Value for it if necessary.
+Value *Scatterer::operator[](unsigned Frag) {
----------------
So this is now "component Frag"?

Although generally it's a little bit messy with terminology. Is a "component" also a "fragment"?


================
Comment at: llvm/lib/Transforms/Scalar/Scalarizer.cpp:424
   } else {
     // Search through a chain of InsertElementInsts looking for element I.
     // Record other elements in the cache.  The new V is still suitable
----------------
I think this should say "looking for element Frag".




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149842