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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1157
+
+  SmallVector<std::pair<LLT, unsigned>> HAdds;
+  if (Ty.isScalar()) {
----------------
paquette wrote:
> doesn't SmallVector need an initial size?
Sean Silva recently added a default to it: https://reviews.llvm.org/D92522


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


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

```
*** Bad machine code: Virtual register does not match instruction constraint ***
- function:    s32_lower
- basic block: %bb.0  (0x7fba998570c8)
- instruction: %ctpop:_(s32) = UADDLVv8i8v %4:_(<8 x s8>)
- operand 0:   %ctpop:_
Expect register class FPR16 but got nothing
```


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1190
+
+  if (Ty.isScalar()) {
+    if (Size == 64)
----------------
paquette wrote:
> Logic here is getting a little complex. Maybe this?
> 
> ```
> if (Ty.isScalar() && Size == 64)
>   MIRBuilder.buildZExt(Dst, UADD);
> else
>   UADD->getOperand(0).setReg(Dst);
> ```
👍


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106388