[PATCH] D27102: [Constants] Add scalable vector support to ConstantVector::getSplat. [IR support for SVE scalable vectors 2/4]

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi Paul,

I'm really not sure what this will give us in benefit. We can't represent a constant vector of unknown lanes without refactoring the IR textual notation (ex. "<n x 4 x i32> <i32 29, ...>"), so why bother replacing a sequence of instructions in SSA form for one all in the same line?

If we don't need this to proceed, and we'd like to change the representation (at least the textual one), I'd rather leave IR changing decisions for later.

cheers,
--renato



================
Comment at: include/llvm/IR/Constants.h:489
   static Constant *getSplat(unsigned NumElts, Constant *Elt);
+  static Constant *getSplat(VectorType::ElementCount EC, Constant *Elt);
 
----------------
Shouldn't you replace the current one, rather than add? Or even, make the old one a wrapper to the EC version with isScalable = false?


================
Comment at: lib/IR/Constants.cpp:1048
+Constant *ConstantVector::getSplat(VectorType::ElementCount EC, Constant *V) {
+  if (!EC.isScalable())
+    return getSplat(EC.getNumElements(), V);
----------------
This is confusing. Better to merge the two and make the other call this one with `{ NumElts, false }`.


================
Comment at: test/Transforms/ConstProp/splat.ll:40
+; CHECK-LABEL: @test6
+; CHECK: ret <n x 4 x i32> shufflevector (<n x 4 x i32>
+; CHECK-SAME:  insertelement (<n x 4 x i32> undef, i32 29, i32 0),
----------------
I'm not sure what we gain by this being all in the same line versus separate lines.


https://reviews.llvm.org/D27102