GlobalISel: Translate ConstantDataVector

[dsanders]

This will LGTM for the architectures supported by GlobalISel at the moment with a couple more tests but I think there's targets in LLVM that need different behaviour (PowerPC?). If that's the case, should we account for this now or later?

lib/CodeGen/GlobalISel/IRTranslator.cpp

988

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?).

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

1193–1201

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'.

[kristof.beyls]

lib/CodeGen/GlobalISel/IRTranslator.cpp

988

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?

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

1193–1201

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.

[ab]

I'm not sure we want to use G_SEQUENCE for this. It seems to me that we should figure out a representation for vectors in general before diving into constants.

lib/CodeGen/GlobalISel/IRTranslator.cpp

988

This isn't always a valid assumption; we can have things like v16i1.

Make this size in bits instead?

[dsanders]

lib/CodeGen/GlobalISel/IRTranslator.cpp

988

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.

[qcolombet]

lib/CodeGen/GlobalISel/IRTranslator.cpp

988

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.

990

For this case we should use G_MERGE.

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

1193–1201

For the IRTranslator non-power-of-2 vectors should be fine.
At least I don't see any difficulty for this specific pass.

[dsanders]

lib/CodeGen/GlobalISel/IRTranslator.cpp

988

I don't think it matters to the SelectionDAG representation but I believe it's important to the transition between SelectionDAG nodes to MachineInstrs. Mips was handling the bitconvert differences at instruction selection time. I'm not familiar with the PowerPC instruction set but it looks like PowerPC is doing some endian aware lowering of BUILD_VECTOR in PPCTargetLowering::LowerBUILD_VECTOR().

I suppose the key question to answer is: Do gMIR's virtual registers hold SelectionDAG values or physical register values?

[volkan]

lib/CodeGen/GlobalISel/IRTranslator.cpp

988

SelectionDAG uses the same order to build vector.

[volkan]

Use G_MERGE_VALUES to represent vectors.

[qcolombet]

LGTM + test case

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

1193–1201

Could you add a non-power-of-2 test case?

[volkan]

Added a non-power-of-2 test.

[ab]

This approach makes sense to me too, thanks folks! If no one else has a paragraph handy, I'll try to explain the whole post-G_SEQUENCE scheme in the docs page.

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

1202–1210

Any reason for this test to involve an extractelement?

[volkan]

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

1202–1210

We are unable to translate ret <3 x i32> <...>, so I tried something else.

lib/CodeGen/GlobalISel/IRTranslator.cpp

bool IRTranslator::translate(const Constant &C, unsigned Reg) {
   else if (auto CF = dyn_cast<ConstantFP>(&C))
     EntryBuilder.buildFConstant(Reg, *CF);
   else if (isa<UndefValue>(C))
     EntryBuilder.buildInstr(TargetOpcode::IMPLICIT_DEF).addDef(Reg);
   else if (isa<ConstantPointerNull>(C))
     EntryBuilder.buildConstant(Reg, 0);
   else if (auto GV = dyn_cast<GlobalValue>(&C))
     EntryBuilder.buildGlobalValue(Reg, GV);
-  else if (auto CE = dyn_cast<ConstantExpr>(&C)) {
+  else if (auto CV = dyn_cast<ConstantDataVector>(&C)) {
+    std::vector<unsigned> Ops;
+    std::vector<uint64_t> Indices;
+    for (unsigned i = 0; i < CV->getNumElements(); ++i) {
+      Constant &Elt = *CV->getElementAsConstant(i);
+      Ops.push_back(getOrCreateVReg(Elt));
+      Indices.push_back(i * (CV->getElementByteSize() * 8));
+    }
+    EntryBuilder.buildSequence(Reg, Ops, Indices);
+  } else if (auto CE = dyn_cast<ConstantExpr>(&C)) {
     switch(CE->getOpcode()) {
 #define HANDLE_INST(NUM, OPCODE, CLASS)                         \
       case Instruction::OPCODE: return translate##OPCODE(*CE, EntryBuilder);
 #include "llvm/IR/Instruction.def"
     default:
       if (!TPC->isGlobalISelAbortEnabled())
         return false;
       llvm_unreachable("unknown opcode");

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

; CHECK: G_STORE [[V2]](s8), [[ADDR]](p0) :: (store singlethread monotonic 1 into %ir.addr)
   %v1 = load atomic i8, i8* %addr acquire, align 1
   store atomic i8 %v1, i8* %addr release, align 1
 
   %v2 = load atomic i8, i8* %addr singlethread seq_cst, align 1
   store atomic i8 %v2, i8* %addr singlethread monotonic, align 1
 
   ret void
 }
+
+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:
+  ret <2 x i32> <i32 1, i32 2>
+}