This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Handle some odd splits in fewerElementsVector
ClosedPublic

Authored by arsenm on Jan 18 2019, 11:42 PM.

Details

Summary

Also add some quick hacks to AMDGPU legality for the tests.

Diff Detail

Event Timeline

arsenm created this revision.Jan 18 2019, 11:42 PM
arsenm updated this revision to Diff 182823.Jan 21 2019, 1:35 PM

Split out unrelated legality changes

Added some inline comments.

I think the biggest thing is that it'd be nice to have some comments breaking up each step in the algorithm into discrete stages. Other than that, this looks fine to me modulo a couple nits.

lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1400

NarrowSize * NumParts is defined as ExtraOffset later, so I guess it might as well be defined here? (I guess ExtraOffset wouldn't be a great name then, though, since it's not really "extra" yet)

1400

I feel like this logic can be tightened up a bit.

// The number of bytes that we'd cover by splitting the vector into NumParts chunks of NarrowSize bytes.
const unsigned BytesForNumParts = NarrowSize * NumParts;

// Check if we have any leftovers. If we do, then only handle the case where the leftover is one element.
if (BytesForNumParts != Size && BytesForNumParts + EltSize != Size)
   return UnableToLegalize;
1408

I think it would be good to have a comment explaining what this loop is doing.

1409

A comment explaining what's going on here would be useful here too, just to make the separate cases clear.

1428

Assertion message would be nice here. E.g

assert(ExtraOffset + EltSize == Size && "More than one element left over?");
1431

Comment would be nice.

arsenm updated this revision to Diff 182972.Jan 22 2019, 1:13 PM
arsenm marked 4 inline comments as done.

Cleanups, add comments

This revision is now accepted and ready to land.Jan 23 2019, 10:54 AM
arsenm closed this revision.Jan 29 2019, 6:22 PM

r352591

dsanders added inline comments.Apr 29 2019, 2:46 PM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1398

Hi Matt,

I just came across the various uses of SrcOp in LegalizerHelper and thought I should mention that this type wasn't really meant to be used outside of MachineIRBuilder and its specializations. It's a bit of machinery to be able to factor out the common overloads.

What was the reason for introducing the SrcOp variables? Does MachineOperand or registers achieve the same end result?

arsenm marked an inline comment as done.Apr 29 2019, 2:54 PM
arsenm added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1398

I think this was because you can't implicitly convert from SmallVector<unsigned> to ArrayRef<SrcOp> for the final use

dsanders added inline comments.Apr 29 2019, 3:06 PM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1398

That makes sense, we should be able to add a conversion for that. In later commits there's also some SrcOp variables that aren't in a container. What was the reason for those?

arsenm marked an inline comment as done.Apr 29 2019, 3:24 PM
arsenm added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1398

In the narrowScalarShiftByConstant for example, I think this was to have an uninitialized variable that the later build calls would set from the MachineInstrBuilder results. It would be a pain if this was a register value, since you don't really want to be referring to registers at all.

dsanders added inline comments.Apr 29 2019, 3:38 PM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1398

Thanks. Given that's the aim, we could probably replace the ones like that with a default-constructed MachineInstrBuilder since that's what the SrcOp there is really holding for its lifetime (except the initial 'uninitialized' portion where it's holding register 0).