[PATCH] D56957: GlobalISel: Handle some odd splits in fewerElementsVector

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

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.



================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1396
+
+  if (NarrowSize * NumParts != Size) {
+    // Only handle the case where the leftover is one element.
----------------
`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)


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1396
+
+  if (NarrowSize * NumParts != Size) {
+    // Only handle the case where the leftover is one element.
----------------
paquette wrote:
> `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)
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;
```


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1404
+
+    for (unsigned Offset = 0; Offset < Size; Offset += NarrowSize) {
+      if (Offset + NarrowSize < Size) {
----------------
I think it would be good to have a comment explaining what this loop is doing.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1405
+    for (unsigned Offset = 0; Offset < Size; Offset += NarrowSize) {
+      if (Offset + NarrowSize < Size) {
+        SmallVector<SrcOp, 4> SrcOps;
----------------
A comment explaining what's going on here would be useful here too, just to make the separate cases clear.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1424
+    const unsigned ExtraOffset = NarrowSize * NumParts;
+    assert(ExtraOffset + EltSize == Size);
+
----------------
Assertion message would be nice here. E.g

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


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1427
+    SmallVector<SrcOp, 4> SrcOps;
+    for (unsigned I = 1, E = MI.getNumOperands(); I != E; ++I) {
+      unsigned PartOpReg = MRI.createGenericVirtualRegister(EltTy);
----------------
Comment would be nice.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56957/new/

https://reviews.llvm.org/D56957