This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Translate ConstantDataVector
ClosedPublic

Authored by volkan on Feb 21 2017, 10:38 AM.

Diff Detail

Event Timeline

volkan created this revision.Feb 21 2017, 10:38 AM
dsanders accepted this revision.Feb 28 2017, 7:01 AM

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

This revision is now accepted and ready to land.Feb 28 2017, 7:01 AM
kristof.beyls added inline comments.Feb 28 2017, 11:32 AM
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 requested changes to this revision.Feb 28 2017, 4:31 PM

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?

This revision now requires changes to proceed.Feb 28 2017, 4:31 PM
dsanders added inline comments.Mar 1 2017, 1:45 AM
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 added inline comments.Mar 7 2017, 12:03 PM
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 added inline comments.Mar 7 2017, 3:40 PM
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 added inline comments.Mar 7 2017, 3:42 PM
lib/CodeGen/GlobalISel/IRTranslator.cpp
988

SelectionDAG uses the same order to build vector.

volkan updated this revision to Diff 90960.Mar 7 2017, 4:24 PM
volkan edited edge metadata.

Use G_MERGE_VALUES to represent vectors.

volkan marked 2 inline comments as done.Mar 7 2017, 4:25 PM
qcolombet accepted this revision.Mar 10 2017, 9:57 AM

LGTM + test case

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1193–1201

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

volkan updated this revision to Diff 91584.Mar 13 2017, 10:24 AM

Added a non-power-of-2 test.

volkan marked an inline comment as done.Mar 13 2017, 10:24 AM
ab accepted this revision.Mar 13 2017, 2:37 PM

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?

This revision is now accepted and ready to land.Mar 13 2017, 2:37 PM
volkan added inline comments.Mar 13 2017, 2:44 PM
test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1202–1210

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

volkan closed this revision.Mar 13 2017, 2:48 PM