[PATCH] D57426: GlobalISel: Implement widenScalar for G_UNMERGE_VALUES

[Amara Emerson via Phabricator via llvm-commits]

aemerson requested changes to this revision.
aemerson added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:792
+  unsigned NumSrc = MI.getNumOperands() - 1;
+  unsigned EltSize = DstTy.getSizeInBits() / NumSrc;
+  unsigned ResultReg = MRI.createGenericVirtualRegister(DstTy);
----------------
Rename EltSize to PartSize to make it clear it's not a vector element.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:797
+  for (unsigned I = 1; I != NumSrc; ++I) {
+    const unsigned Offset = I * EltSize;
+
----------------
`I` to me is the source register index into the instruction that we're inserting, so I think this would be better expressed as:

```
for (unsigned I = 2; I != NumRc; ++I) {
  const unsigned Offset = (I - 1) * EltSize;
```
and then just use `I` in the other two places.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:807
+    unsigned Shl = MRI.createGenericVirtualRegister(DstTy);
+    unsigned NextResult = I + 1 == NumSrc ? DstReg :
+      MRI.createGenericVirtualRegister(DstTy);
----------------
and shouldn't this be I + 2 anyway?


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:855
+
+  MI.getOperand(NumDst).setReg(WideSrc);
+  for (unsigned I = 0; I != NumDst; ++I)
----------------
Since we modify this in place we need to tell the Observer we're changing MI.


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

https://reviews.llvm.org/D57426