[PATCH] D30216: GlobalISel: Translate ConstantDataVector

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:988
+      Ops.push_back(getOrCreateVReg(Elt));
+      Indices.push_back(i * (CV->getElementByteSize() * 8));
+    }
----------------
dsanders wrote:
> kristof.beyls wrote:
> > dsanders wrote:
> > > ab wrote:
> > > > This isn't always a valid assumption; we can have things like v16i1.
> > > > 
> > > > Make this size in bits instead?
> > > This looks correct for ARM/AArch64/Mips, but I'm not sure this is correct for all targets. I'm thinking of big-endian targets where the highest-numbered element is stored at bit 0 (PowerPC?).
> > If this is not correct for some targets, there should at list be a FIXME here explaining in what circumstances the code isn't correct?
> > This isn't always a valid assumption; we can have things like v16i1.
> 
> v16i1 can't occur here. ConstantDataVector handles the common case vectors where the elements are i8/i16/i32/i64/half/float/double.  The v16i1 case is handled by ConstantVector instead.
A genuine question, does SD care about the endian distinction for building vectors?
The reason I am asking is because I could see this being the target dealing with that at selection time, so I want to make sure we are consistent with whatever SD was doing.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:990
+    }
+    EntryBuilder.buildSequence(Reg, Ops, Indices);
+  } else if (auto CE = dyn_cast<ConstantExpr>(&C)) {
----------------
For this case we should use G_MERGE.


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll:1193-1201
+define <2 x i32> @test_constant_datavector() {
+; CHECK-LABEL: name: test_constant_datavector
+; CHECK: [[ONE:%[0-9]+]](s32) = G_CONSTANT i32 1
+; CHECK: [[TWO:%[0-9]+]](s32) = G_CONSTANT i32 2
+; CHECK: [[RES:%[0-9]+]](<2 x s32>) = G_SEQUENCE [[ONE]](s32), 0, [[TWO]](s32), 32
+; CHECK: %d0 = COPY [[RES]](<2 x s32>)
+entry:
----------------
kristof.beyls wrote:
> dsanders wrote:
> > I'd add a couple more tests, one for another vector size (preferably not a power of 2) and one for a different type and size like 'double'.
> My guess is that non-power-of-2 vector sizes are too ill-supported to be able to write tests using them at the moment?
> I'm currently starting to look into supporting non-power-of-2-sized types - it's looking like it may be quite a bit of work.
For the IRTranslator non-power-of-2 vectors should be fine.
At least I don't see any difficulty for this specific pass.


https://reviews.llvm.org/D30216