[PATCH] D77434: [llvm][Codegen] Make `getVectorTypeBreakdownMVT` work with scalable types.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:954
+  // Ideally we could break down into LHS/RHS like LegalizeDAG does.
+  if (!EC.isPowerOf2()) {
+    // Split EC to unit size (scalable property is preserved).
----------------
I'd prefer just making people write out `isPowerOf2_32(EC.Min)`.  This sort of operation should be rare, and I'd prefer to spell out what it actually means.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:974
+  assert(!(EC.Scalable && EC.Min == 1) && "The split has resulted in a"
+                                          "scalable vector with one lane.");
 
----------------
I'm not sure how you expect this assertion to work out.  For the current set of defined MVTs, it might work for SVE, but this will explode as soon as someone adds MVTs for a new target, or tries to add support for a target that doesn't have f16 vectors, or we query this on a target that doesn't support scalable vectors.

There's also the larger problem of how legalization should work if we run into a scalable vector with an illegal element type.  Not sure what the right answer is there.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:986
   // Convert sizes such as i33 to i64.
-  if (!isPowerOf2_32(NewVTSize))
-    NewVTSize = NextPowerOf2(NewVTSize);
+  if (!NewVTSize.isPowerOf2())
+    NewVTSize = TypeSize::getNextPowerOf2(NewVTSize);
----------------
At this point, the element count is known to be a power of two; maybe it would be simpler to work with the element types here?  Those are never scalable, so it's more obvious that NextPowerOf2 does something sane.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77434