[PATCH] D57164: GlobalISel: Handle odd splits in fewerElementsVector for load/store

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.
Herald added a subscriber: Petar.Avramovic.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerHelper.h:135
+
+  void insertParts(unsigned DstReg, LLT ResultTy, LLT PartTy, LLT LeftoverTy,
+                   ArrayRef<unsigned> PartRegs,
----------------
Comment?


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1700
+  unsigned NarrowSize = NarrowTy.getSizeInBits();
+  unsigned NumParts = Size / NarrowSize;
+  unsigned LeftoverSize = Size - NumParts * NarrowSize;
----------------
Maybe we should assert that `Size >= NarrowSize`?


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1742-1743
+    NumParts = getNarrowTypeBreakDown(ValTy, NarrowTy, LeftoverTy);
+  } else {
+    if (extractParts(ValReg, ValTy, NarrowTy, LeftoverTy, NarrowRegs,
+                     NarrowLeftoverRegs))
----------------
Can this just be an else if?


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1748
+
+  if (NumParts == -1)
     return UnableToLegalize;
----------------
Out of curiosity, could `NumParts` ever be 0 for some reason? If so, what would that mean?


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1755
+
+  auto splitTypePieces = [=](LLT PartTy, SmallVectorImpl<unsigned> &ValRegs,
+                             unsigned Offset) -> unsigned {
----------------
Can we have a comment explaining what this lambda is doing, just for a small mental breather? :P


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1762
+      unsigned NewAddrReg = 0;
+      MIRBuilder.materializeGEP(NewAddrReg, AddrReg, OffsetTy, Offset / 8);
+
----------------
Offset/8 is used a few places here, maybe make it a variable?


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1779
+
+  unsigned HandledOffset = splitTypePieces(NarrowTy, NarrowRegs, 0);
+  if (LeftoverTy.isValid())
----------------
Can we have a comment here?


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

https://reviews.llvm.org/D57164