[PATCH] D102073: [TargetLowering] Improve legalization of scalable vector types

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 2 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:619
 
-    // FIXME: Use CONCAT for 2x -> 4x.
-    return DAG.getBuildVector(PartVT, DL, Ops);
-  }
+  if (ElementCount::isKnownLE(PartNumElts, ValueNumElts) ||
+      PartVT.getVectorElementType() != ValueVT.getVectorElementType())
----------------
sdesmalen wrote:
> frasercrmck wrote:
> > sdesmalen wrote:
> > > Should this also return `SDValue()` if `PartNumElts.isScalable() != ValueNumElts.isScalable()` ?
> > Possibly. I was thinking that technically we can widen a fixed-length Value to a scalable Part -- the same way we widen a scalable Value to scalable Part -- using INSERT_SUBVECTOR. Though only if the Part element count is known to be greater than the Value element count, so this would disallow widening v4i32 to nxv1i32, for instance. We definitely can't support widening a scalable to a fixed so we must certainly return `SDValue()` there.
> > 
> > I tried to permit these scenarios in the code, though it's hypothetical since we don't have a target that uses the "mixed" widening.
> > 
> > If you think that all of this is unnecessary then I'm happy to take your suggestion.
> Without a specific use-case in mind, I have a slight preference to avoid widening (or possibly allowing to widen) fixed-width to scalable vectors until there is a real need for it, and until we can write a test-case. By returning SDValue, the code in getCopyTo/FromPartsVector will probably fail earlier on, rather than doing something that may be wrong. Maybe it doesn't really matter, but I guess it's easy to change this code if we do need it at some point.
Fair enough, I have a bit of a habit of doing that. I've added that check now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102073