[PATCH] D30216: GlobalISel: Translate ConstantDataVector

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:988
+      Ops.push_back(getOrCreateVReg(Elt));
+      Indices.push_back(i * (CV->getElementByteSize() * 8));
+    }
----------------
qcolombet wrote:
> 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.
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?


https://reviews.llvm.org/D30216