[PATCH] D72608: GlobalISel: Don't ignore requested ext narrowing type

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:79
+
+  assert(!OrigTy.isVector() && !TargetTy.isVector());
+
----------------
It would be nice to have the assert explain why `OrigTy.isVector() && TargetTy.isVector()` isn't handled.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:248
 
+static void getUnmergeResults(SmallVectorImpl<Register> &Regs,
+                              const MachineInstr &MI) {
----------------
Can we have a comment explaining the purpose of this function?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:261-266
+  if (SrcTy == GCDTy)
+    Parts.push_back(SrcReg);
+  else {
+    auto Unmerge = MIRBuilder.buildUnmerge(GCDTy, SrcReg);
+    getUnmergeResults(Parts, *Unmerge);
+  }
----------------
Can we change the bracing/logic here so that the else isn't the only thing with braces? I think it's fine to put braces around the `if` here.

Also a comment on the else/if cases would be nice.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:309
+
+  Register AllPadReg;
+  for (int I = 0; I != NumParts; ++I) {
----------------
Comment for what `AllPadReg` is?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:310-312
+  for (int I = 0; I != NumParts; ++I) {
+    bool AllMergePartsArePadding = true;
+    for (int J = 0; J != NumSubParts; ++J) {
----------------
Can we have a comment explaining what these loops are doing?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:314-318
+      if (Idx < NumOrigSrc) {
+        SubMerge[J] = VRegs[Idx];
+        AllMergePartsArePadding = false;
+      } else
+        SubMerge[J] = PadReg;
----------------
Can we avoid having the if only have braces here?

e.g.

```
if (Idx >= NumOrigSrc) {
  SubMerge[J] = PadReg;
  continue;
}

SubMerge[J] = VRegs[Idx];
AllMergePartsArePadding = false;
```

Also I'd like a comment explaining why `Idx < NumOrigSrc` sets `AllMergePartsArePadding` to false.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:321-329
+    // If we've filled up a complete piece with padding bits, we can directly
+    // emit the natural sized constant if applicable, rather than a merge of
+    // smaller constants.
+    if (AllMergePartsArePadding && !AllPadReg) {
+      if (PadStrategy == TargetOpcode::G_ANYEXT)
+        AllPadReg = MIRBuilder.buildUndef(NarrowTy).getReg(0);
+      else if (PadStrategy == TargetOpcode::G_ZEXT)
----------------
Comment for why we don't set `AllPadReg` on `G_SEXT`?


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

https://reviews.llvm.org/D72608