Page MenuHomePhabricator

GlobalISel: Implement moreElementsVector for g_insert results
ClosedPublic

Authored by arsenm on Feb 12 2019, 9:37 AM.

Diff Detail

Event Timeline

arsenm created this revision.Feb 12 2019, 9:37 AM
paquette accepted this revision.Feb 13 2019, 9:40 AM

LGTM with question

lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2370

Should we be asserting that the types are vectors for these? Or does it not matter?

This revision is now accepted and ready to land.Feb 13 2019, 9:40 AM
arsenm marked an inline comment as done.Feb 13 2019, 9:42 AM
arsenm added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2370

These seem to allow any types.

One thing I'm sort of thinking about for some vector legalizations is if this is OK or sufficient.

arsenm marked an inline comment as done.Feb 13 2019, 9:57 AM
arsenm added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2370

My current feeling is there is something wrong with the current set of legalization primitives for vectors, but I haven't tried to come up with a coherent description of what yet. The fact that the insert can clobber multiple vector elements seems difficult to deal with. I almost think we need a version of g_insert that handles complete vector elements, and then maybe some kind of elementwise version (where the bit offset is into each vector element) but I haven't fully thought this out yet.

paquette added inline comments.Feb 13 2019, 9:59 AM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2370

These seem to allow any types.

Oh, I meant that if we're calling moreElementsVectorSrc, we should probably assert that we're legalizing a vector source. That function seems to have the assumption that the type we're giving it a vector type.

My current feeling is there is something wrong with the current set of legalization primitives for vectors

I'll need to think about that a bit

arsenm marked an inline comment as done.Feb 13 2019, 10:09 AM
arsenm added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2370

The assert I added in r352636 should catch that, but I guess you could always call the LegalizerHelper directly

arsenm closed this revision.Feb 20 2019, 8:10 AM

r354477