Also add some quick hacks to AMDGPU legality for the tests.
Details
Diff Detail
Event Timeline
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. |
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? |
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 |
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? |
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. |
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). |
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?