[PATCH] D106388: [AArch64][GlobalISel] Legalize ctpop for v2s64, v2s32, v4s32, v4s16, v8s16

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1157
+
+  SmallVector<std::pair<LLT, unsigned>> HAdds;
+  if (Ty.isScalar()) {
----------------
doesn't SmallVector need an initial size?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1177
+
+  Register HSum = CTPOP.getReg(0);
+  MachineInstrBuilder UADD;
----------------
I think `CTPOP` is only used here?  Maybe it's a little cleaner to move it and related stuff down near it.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1184
+    UADD = MIRBuilder.buildInstr(Opc, {HTy}, {HSum});
+    constrainSelectedInstRegOperands(*UADD, *ST->getInstrInfo(),
+                                     *MRI.getTargetRegisterInfo(),
----------------
I don't think you should have to do this here, since we're before register bank selection?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1190
+
+  if (Ty.isScalar()) {
+    if (Size == 64)
----------------
Logic here is getting a little complex. Maybe this?

```
if (Ty.isScalar() && Size == 64)
  MIRBuilder.buildZExt(Dst, UADD);
else
  UADD->getOperand(0).setReg(Dst);
```


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1198
+  }
+  constrainSelectedInstRegOperands(*UADD, *ST->getInstrInfo(),
+                                   *MRI.getTargetRegisterInfo(),
----------------
This shouldn't be necessary since we haven't selected regbanks yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106388